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

Correctly handle fmt skip comments without internal spaces #2970

Merged
merged 4 commits into from Apr 9, 2022

Conversation

siuryan
Copy link
Contributor

@siuryan siuryan commented Mar 28, 2022

Description

Hi, first time contributor here! This PR fixes #2917 . The issue was that fmt skip comments were being handled inconsistently. A formatted version of the comment (e.g. #fmt:skip -> # fmt:skip in comment.value) was being compared to the original comment in the code (e.g. #fmt:skip in leaf.prefix), which would always be False.

Please let me know if there's anything I missed. I left a comment below explaining one of the changes. Also, does this change require a changelog entry or documentation change?

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

src/black/comments.py Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Mar 28, 2022

diff-shades reports zero changes comparing this PR (ffc8672) to main (421383d).


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

@siuryan
Copy link
Contributor Author

siuryan commented Apr 4, 2022

Hi, just wanted to follow up on this PR. Would a maintainer be able to take a look at these changes? Thanks!

@JelleZijlstra JelleZijlstra self-requested a review Apr 4, 2022
Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Actually, this will need a NEWS entry. You can see examples on other open PRs.

@JelleZijlstra
Copy link
Collaborator

JelleZijlstra commented Apr 5, 2022

Unfortunately I triggered another CI failure because I merged main, I'll review the PR fixing that now.

@JelleZijlstra JelleZijlstra self-requested a review Apr 5, 2022
Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Thanks!

It occurs to me that maybe we shouldn't be formatting # fmt: skip comments at all, even adding the space. But I am not going to worry about that until and unless users actually complain.

Copy link
Collaborator

@ichard26 ichard26 left a comment

LGTM, thank you and congrats on your first contribution to psf/black @siuryan! -- always nice to see some new faces :)

@ichard26 ichard26 merged commit 431bd09 into psf:main Apr 9, 2022
40 checks passed
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.

3 participants