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

Possible fix for issue with indentation and fmt: skip #2281

Merged
merged 5 commits into from Jun 8, 2021

Conversation

enzet
Copy link
Contributor

@enzet enzet commented May 30, 2021

Possible fix for #2254

Not sure the fix is right. Here is what I found: the issue is connected with the line

first.prefix = prefix[comment.consumed :]

in comments.py. first.prefix is a prefix of the line, that ends with # fmt: skip, but comment.consumed is the length of the " # fmt: skip" string. If prefix length is greater than 14, first.prefix will grow every time we apply formatting.

@enzet enzet changed the title Possible fix for #2254 Possible fix for issue with indentation and fmt: skip May 30, 2021
@ichard26
Copy link
Collaborator

ichard26 commented Jun 1, 2021

Hey,

I just want to let you know that we should be able to get started on the review process for your changeset soon. Apologies for the wait. Your fix looks promising, but I'm bad when it comes to AST mangling code so take my opinion with a grain of salt :)

Looking forward to reviewing your work, thanks for working on this!

enzet added 2 commits June 3, 2021 00:35
Not sure the fix is right.  Here is what I found: issue is connected
with line

    first.prefix = prefix[comment.consumed :]

in `comments.py`.  `first.prefix` is a prefix of the line, that ends
with `# fmt: skip`, but `comment.consumed` is the length of the
`"  # fmt: skip"` string.  If prefix length is greater than 14,
`first.prefix` will grow every time we apply formatting.
Test for fmt: skip comment when indentation needs more than 14 spaces.
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.

This looks right and the tests look good, but I don't know this part of the code well.

Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in roughly the same boat as Jelle, but I did spent a few hours somewhat recently in this specific part of the codebase trying to fix a bug (I wasn't successful) and this looks good to me as well.

Thank you for the PR! It's awesome to see one more bug fixed. This project is only possible by contributions like these 🖤. You're awesome, @enzet! Perhaps we should have done some more testing before rolling out this new feature 😅. Anyway, it would be even more awesome if you could provide some feedback on your experience contributing to psf/black. No pressure to do so, but it would be helpful. More details here: #2238. Thanks again.

@JelleZijlstra JelleZijlstra merged commit 40fae18 into psf:main Jun 8, 2021
@enzet
Copy link
Contributor Author

enzet commented Jun 9, 2021

Thank you, @ichard26! Hope the fix is helpful.

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