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

# fmt skip internal error fix (space between # and f) #2682

Closed
wants to merge 3 commits into from

Conversation

andrerod22
Copy link

@andrerod22 andrerod22 commented Dec 10, 2021

Description

Bug report: #2680
Test that replicates bug (see fmtskip7.py in tests/data):

a, b = 1, 2
c =   6 #fmt: skip
d = 5

Resolve: It's seen that of all the formatting options, the inline option, or # fmt skip causes an internal error for black, when the comment should be reformatted like # fmt off. A proposal for this issue was made in the fmtskip_nospace branch which aims to hard code logic handling this bug within the make_comment function of comments.py.

Note: This does NOT handle arbitrary spacing for the # fmt: skip which should be handled in another pull request.

Issues with current pull request: Building on different platforms fails due to the unit test fmtskip7.py failing when running pre-config. An investigation is further needed to resolve this issue.

Checklist

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary? See fmtskip7.py in tests/data
  • Add new / update outdated documentation?

@felix-hilden
Copy link
Collaborator

Thanks for the PR! For housekeeping, the list appears as checkboxes if you leave an empty space for "no" and put in an "x" for "yes".

Some points: did you run tests before submitting? Now your test is failing, at least in the CI. So I'm guessing it does not work locally either. Also for the record, I'd like to take care of the multi-space case while we're at it. But I'll be fine if other maintainers just want to push this in.

@andrerod22 andrerod22 changed the title Pre-config run for prev commit. # fmt skip internal error fix (space between # and f) Dec 17, 2021
@andrerod22
Copy link
Author

Looking back at the unit test that failed, the error is within _assert_format_equal
Specifically, the expected output is the following:
image

And the actual output differs here
image

Notice how the NAME variable in the expression statement is different. It seems as though, Black is expecting just the variable I had written on the same line and completely ignores the #fmt: skip comment on the same line. This is strange behavior and I will bring it up in the discord later.

@JelleZijlstra
Copy link
Collaborator

Any updates here? I agree with @felix-hilden that it'd be good to handle other spacing edge cases at the same time.

@github-actions
Copy link

github-actions bot commented Dec 18, 2022

diff-shades reports zero changes comparing this PR (0ed191b) to main (0abe85e).


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

@JelleZijlstra
Copy link
Collaborator

It appears #2970 already fixed this crash in a more general way, so I'm going to close this PR. Thanks for your contribution!

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.

None yet

3 participants