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
PEP 639: Implement License-Expression and License-File #828
base: main
Are you sure you want to change the base?
PEP 639: Implement License-Expression and License-File #828
Changes from all commits
de8b239
44beda7
2b1a9f7
9bfa714
1aea212
cec33f1
cfb3af1
d6b47d5
a55f422
afa5d4c
396e4ef
21a2821
4ac18f0
46a7491
cd7105f
e469b7e
e699391
22fa9cd
f952ab9
30e34f1
8906b16
a361294
9cee38e
81efbda
701217b
4539543
64d3647
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Could globals and locals be passed in here? That feels safer, regardless if it is or not.
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.
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.
I would recommend against this one, this repository doesn't appear to have many stylistic rules enabled but in Hatch (where this came from) I enable the one where control flow statements require no
elif
. In this example, thecontinue
is the statement so the following branch should start anew.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.
I know it isn't necessary, but I have been bit by people not noticing it wasn't an
elif
when changing something like this to not short-circuit the overallif
block.Anyway, I'm guessing you care because you want to copy the code back into Hatch?
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.
I'm going to use whatever lands here and get rid of my custom code now that it's upstreamed. I just wanted maintainers to be aware that when you start using Ruff more it will catch this line, and I think for good reason. Feel free to do whatever you think is best!
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.
I'm a fan of the simpler structure; the
el
isn't necessary, and adds extra complexity (especially when nested). Changing a control flow statement into a non-control flow statement should change control flow, trying to proactively protect against it isn't helpful, IMO.And this is called a "guard pattern" - it's even part of the syntax in Ruby. (It would be
continue if normalized_tokens and normalized_tokens[-1] == "WITH"
if Python had Ruby's guard, btw).