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

Fix "# pyright: ignore" comment mobility #3661

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

Conversation

etripier
Copy link

@etripier etripier commented Apr 26, 2023

Description

Hi! It looks like you have some special handling for # type: ignore comments to avoid moving said comments around due to positional meaning. I think this same special handling should also apply to # pyright: ignore comments.

The use case for # pyright: ignore comments is being able to ignore Pyright-specific diagnostic rules that might not make sense in the context of more generic # type: ignore comments.

Checklist - did you ...

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

@etripier etripier force-pushed the etripier--fix-pyright-ignore-mobility branch from dd659aa to 03600c9 Compare April 26, 2023 19:17
@JelleZijlstra
Copy link
Collaborator

Should we add this to black.comments.contains_pragma_comment instead? I think these pyright comments are more like noqa than like # type:, which is treated specially by the Python parser.

@etripier
Copy link
Author

Ah, my understanding is that the pragma stuff only applies to string concatenation and splitting. I'm really hoping to fix the annoyance of having

blah_blah_blah = [ # pyright: ignore
    ...
]

shift into

blah_blah_blah = [
    ...
] # pyright: ignore

The behavior in

def contains_unsplittable_type_ignore(self) -> bool:
is what I'd love to have, ideally.

src/black/nodes.py Outdated Show resolved Hide resolved
Co-authored-by: James Hilton-Balfe <[email protected]>
@roshanjrajan-zip
Copy link

roshanjrajan-zip commented Apr 29, 2023

Hi! I wanted to see if there any concerns about gettin this merged other than fixing the conflicts?

If it isn't clear, we get scenarios where black transform code from this

some_long_variable_name = some_long_dict_name[0] # pyright: ignore[reportGeneralTypeIssues]

to this

some_long_variable_name = some_long_dict_name[
	0
] # pyright: ignore[reportGeneralTypeIssues]

where after the transformation, the ignore is no longer being applied to the actual line causing pyright to error. This looks similar to the # type: ignore fix and this would helpful to have! Let me know if you have any other questions!

Our current solution is use cast or do a rename that looks like this

a = some_long_dict_name[0] # pyright: ignore[reportGeneralTypeIssues]
some_long_variable_name = a  # pyright: ignore[reportGeneralTypeIssues]

which are both not ideal scenarios and is very tedious and adds unnecessary bloat to the codebase.

Also using # fmt: skip doesn't work because sometimes it is ignored and moves the comment anyways.

@roshanjrajan-zip
Copy link

roshanjrajan-zip commented May 3, 2023

@JelleZijlstra
In this case, is it better to do change it in the contains_pragma_comment to get the intended behavior or does it make sense to keep it in is_type_comment?

@JelleZijlstra
Copy link
Collaborator

I need to look more into this to figure out exactly how we use these, but a few thoughts:

  • I think we should treat the pyright comments more like the pylint comments than like the # type: ignore comments. # type: comments are extra special because they are treated specially by the Python AST, so it makes sense to be even more conservative with them.
  • I'd prefer to find a solution that avoids Black having too much knowledge of magic comments used by specific tools, since that's bad for long-term maintainability.

@etripier
Copy link
Author

etripier commented May 3, 2023

Would it make more sense to add a Black configuration option opting certain types of patterned comments into the unsplittable behavior, then?

@vors
Copy link

vors commented Jun 25, 2023

Would love to see this feature, it's much needed.

@vors
Copy link

vors commented Jun 25, 2023

type: comments are extra special because they are treated specially by the Python AST, so it makes sense to be even more conservative with them

For my education, can you please elaborate how they are treated differently?

@vors
Copy link

vors commented Jul 8, 2023

Looks like the best path forward for our company at the moment is to fork black and apply this patch. We really need this functionality soon. This is very unfortunate, I hope there is a path forward in upstream to resolve this.

@vors
Copy link

vors commented Jul 8, 2023

Actually just tried applying the patch on 23.3.0 with python 3.9 and it doesn't seem to change anything... Maybe that's what you referred to @JelleZijlstra ?

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.

5 participants