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
Allow no space in doublestar when math operation (#538) (#2095) #2095
Allow no space in doublestar when math operation (#538) (#2095) #2095
Changes from 1 commit
3588ab1
4d81ab9
2d346e6
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.
I like that more verbose wording too! And since it's not updated yet let's grammar-nazi it up just a wee bit: "unless it's the only operation". First I thought it read something like "unless it's just operation" which confused me a bit. Obvious on closer inspection but worth a correction in my mind 😄
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.
Damn this should have been a reply to @ichard26, see his suggested changes above.
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 little concerned by the performance impact this will have. Doing some rough testing, formatting
src/black/__init__.py
spends ~6500ms in theformat_str
method and ~4% (~300ms) of the total runtime was spent in thefix_doublestar_in_op
... which is IMO too much for such a simple case like this.Profiling data (warning: extremely large image)
Is it possible to make this more efficient? Cloning the leaf seems to be where most of 300ms cost is coming from. Perhaps moving the logic somewhere into one of the transformers called in
transform_line
could help since some lines might not need this fixing? I'm sorry for my unadvice, I usually don't review formatting code.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.
Ohh okay, gonna take a look. Thanks for pointing out!
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.
Agreed with @ichard26, it would be cleaner if it were a StringTransformer like the other transformation. This would additionally solve the performance issue. Few enough lines in Python have DOUBLESTAR tokens that the simplest way to improve performance is just look whether there even is doublestar on the line in
do_match()
.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.
Should also test what happens if there is a space in the input.
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.
Maybe is it more a naming convention for the new option, maybe
skip
the chosen name is not the best, and I can change it (I assume update to PRs are allowed in code reviews). Regarding to the spaces question, it will yield the same output (meaning,DOUBLESTAR
without spaces).Does your comment mean, update the code and not merge yet, or does it mean: These changes are not convenient, forget about them?
I am sorry if I don't understand correctly it is my first PR on an open-source repo.
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.
Not sure, but I think Jelle meant you should add test cases which have spaces around the operators, which I think you've done in the code just below! @JelleZijlstra what do you reckon?
I'm not sure about the test writing practices of Black, but I see lots of tests being written a bit more condensed. In your file it's nice to see immediately what's expected from the function name, but another option would be to simply have all the cases in one succession. That's up for debate 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.
That I like.
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.
Same, looks good.
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.
Very nice.
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.
Wait... what? You really think this is more readable in the new form? I think that once you hit a complex base or exponent, then there is no value in skipping the spaces.
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 don't see anything that would make this formatting Python 3.6+ only. I'm pretty sure using
DEFAULT_MODE
is fine here, and for such a simple and common test, there's special tooling for it.black/tests/test_format.py
Lines 16 to 63 in 6d0bdc2
Just add an item of
"doublestar_no_spaces"
and your test should be picked up!