-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
fix: bug where the doublestar operation had inconsistent formatting. #4154
Conversation
Hi! Seems like diff-shades check failed with "6 semaphore objects missing" user warning. From my experience this was a result of memory issue rather than the test actually failing. Could we try running this step again? Thanks! |
Thanks! I haven't looked in detail yet, but this likely needs to go into the preview style only if it changes formatting for existing code. (diff-shades didn't find any, but that might just be because the patterns where this makes a difference are rare.) |
Review would be appreciated! |
Thanks, I'm planning to look at this after the 24.1 release goes out (hopefully today/tomorrow). There's enough in that release already :) |
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.
Thanks!
This will need to go in the preview style, so gated behind an if Preview... in mode
check (you can see examples elsewhere in the code).
Hi @JelleZijlstra, I have addressed the comments, could you kindly review again? 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.
Thanks! Getting very close.
…ber it is trying to fix
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
…Also update test case to use latin letters
for more information, see https://pre-commit.ci
e56874d
to
f514570
Compare
for more information, see https://pre-commit.ci
@JelleZijlstra Thanks for the review! Addressed the comments hope this works out :) |
Description
Fixes #4149, which according to the ternary operation expression was not correctly being formatted. This was caused by the existing black's logic to determine whether the
base
of the exponential expression issimple
or not, meaning whether it does not have parenthesis or brackets and such and just accepting naming or dotted lookups. Essentially, the existing logic was to loop over the direction (base backwards, exponential forward) and find whether a bracket or parenthesis existed. This logic worked for theexponential
case since it doesn't need to care about chained expressions. This PR is to involve checking whether base is a chained expression and check the expression is simple.Just to make sure, I added some more complex cases that would cause the bug other than the case mentioned in the issue.
Checklist - did you ...
CHANGES.md
if necessary?