-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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: Consistently add trailing comma on typed parameters #4164
Conversation
Signed-off-by: RedGuy12 <[email protected]>
diff-shades results comparing this PR (0871f82) to main (ed770ba). The full diff is available in the logs under the "Generate HTML diff report" step.
|
Signed-off-by: RedGuy12 <[email protected]>
leaf.type == token.COMMA | ||
and ( | ||
Preview.typed_params_trailing_comma not in original.mode | ||
or not is_part_of_annotation(leaf) |
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.
Can we use is_part_of_annotation
to simplify some of the logic in the next check too? Lines 1092-1099?
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 tried, but it ended up adding a lot of unrelated changes to this PR since is_part_of_annotation
currently checks for both return and parameter annotations. I can do that refactor in a future PR.
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.
Thank you for all you recent PRs!
Signed-off-by: RedGuy12 <[email protected]>
No idea why diff-shades is detecting changes in stable style now, I only merged upstream in and didn't change any logic. Additionally, it's only finding changes when linting Black itself, past diff-shades runs showed changes in other projects on preview style (compare stable run today and preview run Wednesday) |
I suspect it's because Black itself is now formatted with |
ichard26/diff-shades#15 as a quick patch |
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.
Looks good. I'll fix the one minor issue, then hopefully diff-shades will succeed if I retry.
Maybe ichard26/diff-shades#16 helps, trying again. |
diff-shades succeeded (not sure it's because of my changes, as it was apparently already using 4 processes). Quite a few changes, but they all look expected. |
Description
Resolves #4080
Checklist - did you ...
CHANGES.md
if necessary?