-
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
Format type hints after comments #3362
base: main
Are you sure you want to change the base?
Conversation
diff-shades results comparing this PR (029fce9) to main (3a2d76c). The full diff is available in the logs under the "Generate HTML diff report" step.
|
Sorry, I should have run diff-shades before submitting a pull request. I'll keep working on this, and see if I can fix this solution or think of a better one. |
It's fine to rely on CI to run diff-shades for you. I haven't looked closely here but probably you just need to gate your changes to the preview style. |
I think I've got a better solution now. If there are comments at the beginning or end of the line, I call standalone_comment_split before delimiter_split. Else, I call delimiter_split first. I'm not using the comma_split function I wrote any more. This means that now, when there is a comment in the middle of an expression or list, it will perform a delimiter split. (
a
and b
# comment
and c
and d
) But when the comment is at the top or bottom, it won't perform a delimiter split (if it doesn't have to). (
# comment
a and b and c and d
) Currently, Black performs a delimiter split in both cases, which is unnecessary. |
Should I rebase main onto here to resolve the merge conflict, or will the maintainer do that? The only merge conflict is one line in CHANGES.md. |
Easiest to fix simple merge conflicts by going through GitHub's UI, I just did it for you. |
Okay, thanks. |
@charlie572 I'm interested in this PR. Can you please rebase? |
Great. I can do this tomorrow. Do I just need to rebase main onto this branch to fix merge conflicts? |
# Conflicts: # CHANGES.md # src/black/mode.py
This test makes sure a trailing comma is added when there is a comment in the brackets. However, a delimiter split is no longer performed in these cases, so there is no need to add a trailing comma. This commit makes the lines longer so a delimiter split is performed, and the behaviour with the trailing comma can be tested.
Looking at the diff shades report https://github.com/psf/black/actions/runs/5431147643/jobs/9880366015?pr=3362 it looks this causes the "magic" trailing comma to not get respected inside collection literals if there's a comment |
I'm confused about the magic_trailing_comma attribute of the Line class. Why does it point at the closing bracket after the magic trailing comma, and not at the magic trailing comma token? This means that my code can't check for the magic trailing comma, since it is looking at a Line object containing just the comma-separated members, not the surrounding brackets. I can think of two ways to make it handle magic trailing commas properly: Option a makes the most sense to me. Is that okay? |
I implemented option a. But now the attribute names are weird. Maybe magic_trailing_comma should point at the comma, and bracket_after_magic_trailing_comma can point at the following bracket? Or maybe we can remove the latter attribute? |
Magic_trailing_comma now points to the actual comma token, and bracket_after_magic_trailing_comma now points to the following close bracket.
@charlie572 I understand nothing from the code in this PR (I'm not familiar with the internals of Black) but I want to thank you for your effort. |
Thank you. |
Description
This fixes #3350.
The problem is that when Black encounters standalone comments on one line inside brackets, it splits the line using delimiter_split. This will split on the comment, but it will also split the line on the highest priority delimiter, which isn't always necessary.
I've added a comma_split function, which is the same as delimiter_split, but
delimiter_priority = COMMA_PRIORITY
. Black will now try comment_split before it tries delimiter_split. This means it will first split on standalone comments and commas, and only split on other delimiters if the lines are still too long after that.Checklist - did you ...
CHANGES.md
if necessary?