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

Add Y062: Forbid duplicates in Literal[] slices #449

Merged
merged 11 commits into from
Nov 15, 2023

Conversation

tomasr8
Copy link
Contributor

@tomasr8 tomasr8 commented Nov 12, 2023

Previous discussion for context: #435 (comment)

Example:
Literal[True, True, False, False] - emits Y062 for both True and False.

This is how the new rule interacts with Y061:

  • Literal[None], Literal[None, None], etc.. - Only Y061 is emitted and the suggestion is always just None (previously Literal[None, None] would suggest Literal[] | None)
  • Combination of None and other duplicates e.g. Literal[None, True, True] - emits Y061, which suggests Literal[True, True] | None, and a separate Y062. The Y061 suggestion is not entirely correct though. In theory we could provide a better one (i.e. Literal[True] | None) but I'm not sure if we should, given that we emit a separate Y062 as well.. Thoughts?

@tomasr8 tomasr8 changed the title Literal dupes Add Y062: Forbid duplicates in Literal[] slices Nov 12, 2023

This comment has been minimized.

Copy link
Collaborator

@AlexWaygood AlexWaygood 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 great; I really like the way you put it together :)

tests/literals.pyi Outdated Show resolved Hide resolved
pyi.py Outdated Show resolved Hide resolved

This comment has been minimized.

@tomasr8
Copy link
Contributor Author

tomasr8 commented Nov 13, 2023

Wow that's a hell of a review 😮 Thanks for the taking the time to write it all up! I had hunch the py38 fails would have something to do with the AST changes but wasn't sure so that cleared that up :) I ended up removing the offending doctest (agreed that it's probably not worth to try to fix it) and applied your suggestion to suppress Y061 when we emit Y062.

This comment has been minimized.

pyi.py Outdated Show resolved Hide resolved

This comment has been minimized.

@tomasr8
Copy link
Contributor Author

tomasr8 commented Nov 15, 2023

From my side, this should be gtg ;)

pyi.py Show resolved Hide resolved
pyi.py Show resolved Hide resolved
tests/literals.pyi Outdated Show resolved Hide resolved
Copy link
Collaborator

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Nearly there! Left a smattering of minor suggestions :D

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@tomasr8
Copy link
Contributor Author

tomasr8 commented Nov 15, 2023

Applied! The test cases are much better with an actual explanation

This comment has been minimized.

ERRORCODES.md Outdated Show resolved Hide resolved
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Copy link
Collaborator

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks! :D

Copy link

This change has no effect on typeshed. 🤖🎉

@AlexWaygood AlexWaygood merged commit 70aa7a3 into PyCQA:main Nov 15, 2023
20 checks passed
@tomasr8 tomasr8 deleted the literal-dupes branch November 15, 2023 18:43
@AlexWaygood
Copy link
Collaborator

Minutes after merging, I realised that we give completely incomprehensible suggestions for something like this:

from typing import Literal

x: Literal[None] | Literal[None, True]

Output from linting this:

foo.pyi:3:4: Y030 Multiple Literal members in a union. Use a single Literal, e.g. "Literal[None, None, True]".
foo.pyi:3:12: Y061 None inside "Literal[]" expression. Replace with "None"
foo.pyi:3:28: Y061 None inside "Literal[]" expression. Replace with "Literal[True] | None"

I don't think it's a massive issue, though, as this is a real edge case. Definitely not worth fixing if it would mean significantly complicating our code

@tomasr8
Copy link
Contributor Author

tomasr8 commented Nov 16, 2023

Oh damn why are there some many edge cases every time.. 😆 But yeah, as you say, this one seems very uncommon so I'd leave it as is. If it becomes an issue later, we can revisit this..

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

2 participants