Skip to content
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

Merged
merged 15 commits into from Feb 5, 2024

Conversation

wannieman98
Copy link
Contributor

@wannieman98 wannieman98 commented Jan 16, 2024

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 is simple 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 the exponential 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 ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

@wannieman98 wannieman98 marked this pull request as ready for review January 16, 2024 09:47
@wannieman98
Copy link
Contributor Author

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!

@wannieman98 wannieman98 changed the title Fix a bug where the doublestar operation had inconsistent formatting. fix: bug where the doublestar operation had inconsistent formatting. Jan 16, 2024
CHANGES.md Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Jan 17, 2024

diff-shades reports zero changes comparing this PR (d9d1279) to main (7edb50f).


What is this? | Workflow run | diff-shades documentation

@JelleZijlstra
Copy link
Collaborator

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.)

@wannieman98
Copy link
Contributor Author

Review would be appreciated!

@JelleZijlstra
Copy link
Collaborator

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 :)

Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a 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).

tests/data/cases/power_op_spacing.py Outdated Show resolved Hide resolved
tests/data/cases/power_op_spacing.py Outdated Show resolved Hide resolved
tests/data/cases/power_op_spacing.py Outdated Show resolved Hide resolved
src/black/trans.py Outdated Show resolved Hide resolved
src/black/trans.py Outdated Show resolved Hide resolved
src/black/trans.py Outdated Show resolved Hide resolved
src/black/trans.py Outdated Show resolved Hide resolved
@wannieman98
Copy link
Contributor Author

Hi @JelleZijlstra, I have addressed the comments, could you kindly review again? Thanks!

Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a 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.

src/black/trans.py Outdated Show resolved Hide resolved
src/black/mode.py Show resolved Hide resolved
@wannieman98
Copy link
Contributor Author

@JelleZijlstra Thanks for the review! Addressed the comments hope this works out :)

@JelleZijlstra JelleZijlstra merged commit 32230e6 into psf:main Feb 5, 2024
46 checks passed
@wannieman98 wannieman98 deleted the issue_4149 branch February 7, 2024 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent formatting of ** in tenary expression.
3 participants