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 Y036 code to check for possible usage of PEP 604 new syntax #184

Closed
wants to merge 0 commits into from

Conversation

hoefling
Copy link
Contributor

Closes #45.

@AlexWaygood this is roughly what I had in mind when suggesting a new code, just a simple Use PEP 604 union types instead of {Optional,Union} message. @srittau unfortunately, the check for just importing Optional/Union seems to be not enough, since then any typing.Optional/typing.Union usages are skipped. But luckily the _check_import_or_attribute method already covers that 🤞

@hoefling
Copy link
Contributor Author

@srittau @AlexWaygood if you agree on the general idea, I ask for help on how to proceed with the failing tests. I can just disable the new Y036 code if explicit Optional/Union usages are desired, or replace them with PEP 604 types right away.

@Akuli
Copy link
Collaborator

Akuli commented Feb 17, 2022

It's probably best to disable the new Union warning in other unrelated tests, especially when the tests make sure that something works with both kinds of unions. They can be suppressed like this:

(env) akuli@akuli-desktop:~/flake8-pyi$ python3 -m flake8 tests/forward_refs.pyi 
tests/forward_refs.pyi:1:1: Y036 Use PEP 604 union types instead of typing.Optional.
tests/forward_refs.pyi:1:1: Y036 Use PEP 604 union types instead of typing.Union.
(env) akuli@akuli-desktop:~/flake8-pyi$ python3 -m flake8 --ignore=Y036 tests/forward_refs.pyi 
(env) akuli@akuli-desktop:~/flake8-pyi$ 

So you can add a line # flags: --ignore=Y036 to the beginning of the test files. Comments like this are parsed in tests/test_pyi_files.py.

@hoefling
Copy link
Contributor Author

@Akuli awesome, thank you! It was even easier than I expected :-)

@Akuli
Copy link
Collaborator

Akuli commented Feb 18, 2022

There's currently lots of old-style unions in typeshed, and we can't get rid of them before python/mypy#11098 is fixed. I think we should ignore the new error in typeshed's .flake8.

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.

Thanks for the PR — I love the overall idea! A few thoughts:

  1. There are a number of mypy bugs regarding PEP 604 unions that still prevent us using PEP 604 syntax everywhere in typeshed. That means we'd have to disable this check in the typeshed .flake config file for now. That's fine, but I worry about non-typeshed users: it feels a little unfair to raise an error in flake8 if a user tries to import Optional, when the user might have a very good reason to want to import Optional if it's the only way around one of the mypy bugs.
  2. Can we improve the error messages? Typeshed gets a lot of contributors who are fairly new to typing, and won't know what PEP 604 is. The error messages don't have to be dynamic, but I'd like to see something like e.g. use "int | None" instead of "Optional[int]" for Optional, and e.g. use "int | str" instead of "Union[int, str]" for Union.
  3. Please add a short description of the new error code to the README and the CHANGELOG :)

@hoefling
Copy link
Contributor Author

@AlexWaygood @Akuli thinking out loud: maybe I am unnecessarily rushing with opening this PR? I guess I was too carried away by the fix finally being proposed for python/mypy#11098, wanted to sync flake8-pyi with it. Thus totally fine with keeping this unmerged for the time being.

@AlexWaygood
Copy link
Collaborator

@AlexWaygood @Akuli thinking out loud: maybe I am unnecessarily rushing with opening this PR? I guess I was too carried away by the fix finally being proposed for python/mypy#11098, wanted to sync flake8-pyi with it. Thus totally fine with keeping this unmerged for the time being.

I'm very happy to leave this open for now, and merge it as soon as the mypy issues are resolved, if you're happy with that? 🙂 I think this is definitely the way to go as soon as the mypy issues are resolved, for sure!

I'd like to hear what other maintainers think as well, though.

@AlexWaygood
Copy link
Collaborator

python/typeshed#7493 appears to have cut the number of typeshed hits in half — we're now down to around 90 problematic lines in typeshed (many of which flake8-pyi complains about twice, due to Union and Optional being imported on the same line). So... We're getting there!

@AlexWaygood
Copy link
Collaborator

I'm so sorry -- I was trying to merge master into your PR, and I appear to have screwed it up completely ☹️

@AlexWaygood
Copy link
Collaborator

I couldn't reopen the PR, so I've created a new one here: #202

@hoefling
Copy link
Contributor Author

hoefling commented Apr 6, 2022

@AlexWaygood happens, no worries 😎

@JelleZijlstra
Copy link
Collaborator

I turned on my new favorite GitHub option "Always suggest updating pull request branches" for this repo, that should make updating branches easier.

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

Successfully merging this pull request may close these issues.

Suggest using PEP 604-style union types
4 participants