Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adopting
ruff
formatter #7930Adopting
ruff
formatter #7930Changes from 15 commits
142876d
3061734
0d5a88a
44959bc
aa02e81
fe237ff
f0c8dd4
e8598a4
d5e5795
4b8c2d7
03d21de
1a9924e
8cb3a46
b1c15ca
21338f1
c36a29a
7def92f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change also seems odd to me, as it's not changing the last line. I wonder what happens when we use:
In the
pyproject.toml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it should be
'double'
by default, so I don't think that'll change anything. This modification still strikes me as odd though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea good eye there. Let me try the change out and see what happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, your diligence is impressive and I appreciate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like
is already implemented on line 179:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I'll leave this as unresolved for now given that I'm still curious about the change the
ruff
formatter made, but I don't think it's a problem in terms of theflake8-quotes
configuration.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This strikes me as a bug -- we should be using double quotes for multi-line strings regardless of the formatter quote setting. Will look into it shortly and report back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last line doesn't get changed because changing the quote style would require escaping the
'
. See my other comment below.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, as usual, Micha is right -- this is an implicit concatenation, not a multiline string. So it's trying to use single quotes for each segment, and then skipping those that contain a single quote.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. This happened a few times with implicit concatenations, and I thought the pattern was odd at first glance bc it always seemed to be skipping the last line, but that seems to always be where we have a single quote based on the nature of many of our strings. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charliermarsh if you are looking for feedback, I'd think that it would be preferable to use single quotes on the outer level when single quotes are preferred and the string contains both single and double quotes. But I definitely don't care enough to hold off on merging just for this issue 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree — this one struck me as odd. Gonna look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
astral-sh/ruff#8264
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearly it's having a hard time with this as well 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that is weird. I wonder what it's tripping on...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what's happening here is that it's trying to convert each segment to single quotes (as per
quote-style = 'single'
). It's succeeding for the first segment in this implicit concatenation, but it avoids changing the second segment, because that segment itself contains a single quote (and so that single quote would need to be escaped).Arguably we should be making that decision across the entire implicit concatenation (i.e., looking across all segments) to avoid these mixed quotes. I'll file an issue and we'll discuss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ruff changes the quotes of the first string but not of the second because it would require escaping the
'
. Is it unexpected that Ruff uses different quotes for different parts of the implicit concatenated string? Would you prefer if it would change the quotes consistently for the entire implicit concatenated string?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks for the explanation. Clearly I missed the pattern of escaping
'
that was at the root of many of my questions.I do feel like changing quotes consistently for an entire implicit concatenated string might look a bit cleaner, but I'm not adamant about the change. Excited to hear what others think.
@charliermarsh, thanks for filing an issue 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charliermarsh could you link the issue here? I may have just missed it, but didn't see it at a first glance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies -- just filed it here: astral-sh/ruff#8265