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

Don't hug parens if it would mean putting two # type: ignore comments on the same line #4037

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

henriholopainen
Copy link
Contributor

@henriholopainen henriholopainen commented Nov 9, 2023

Description

Small refactor + fixes #4036

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?

@henriholopainen
Copy link
Contributor Author

Hmm, this might have been too hasty. The problem isn't # fmt: ignore on the first line, but instead it is having multiple of those. Will fix proper.

@henriholopainen henriholopainen marked this pull request as draft November 9, 2023 15:46
@henriholopainen henriholopainen changed the title Don't hug parens when # type: ignore comment on the opening line Don't hug parens if it would mean putting two # type: ignore comments on the same line Nov 10, 2023
@henriholopainen
Copy link
Contributor Author

The type: ignore comments don't actually do anything here. The ast_parser is only interested in the amount of TypeIgnores, not which line they live on. So we want to block paren hugging if two type: ignore comments would end up on the same line, as this would be registered as only one TypeIgnore by the ast_parser.

@henriholopainen henriholopainen marked this pull request as ready for review November 10, 2023 14:26
Copy link

diff-shades reports zero changes comparing this PR (3c87261) to main (5773d5c).


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

@JelleZijlstra
Copy link
Collaborator

There's a scary-looking merge conflict, I think because I just merged #4012. Could you take a look?

@henriholopainen
Copy link
Contributor Author

There's a scary-looking merge conflict, I think because I just merged #4012. Could you take a look?

Seems to not be only the merge conflict, the new code also breaks tests from this branch. I'll have to take a closer look at some point.

@JelleZijlstra
Copy link
Collaborator

@henriholopainen have you had a chance to take another look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants