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

Enable TypeAlias check by default #101

Merged
merged 6 commits into from Jan 19, 2022
Merged

Enable TypeAlias check by default #101

merged 6 commits into from Jan 19, 2022

Conversation

JelleZijlstra
Copy link
Collaborator

And delete the machinery for disabling errors.

This made this check run on all tests, which helped me find a bug
(it triggered on all). I can split the test changes into a separate
PR if preferred.

I'll submit a separate PR to typeshed to disable this check for now.

Fixes #75. Fixes #86.

And delete the machinery for disabling errors.

This made this check run on all tests, which helped me find a bug
(it triggered on __all__). I can split the test changes into a separate
PR if preferred.

I'll submit a separate PR to typeshed to disable this check for now.

Fixes #75. Fixes #86.
JelleZijlstra added a commit to python/typeshed that referenced this pull request Jan 19, 2022
@JelleZijlstra
Copy link
Collaborator Author

The typeshed-primer failures are all Y026. We'll have to merge python/typeshed#6958 first for CI to become green.

hauntsaninja pushed a commit to python/typeshed that referenced this pull request Jan 19, 2022
@JelleZijlstra JelleZijlstra reopened this Jan 19, 2022
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.

Looks good, but I think we should maybe advertise this change more loudly, either in the README or the changelog (and maybe point to instructions elsewhere about how to disable certain codes in your flake8 config file?). If third-party users of this package are anything like typeshed, this check is going to cause a large number of hits.

@hauntsaninja
Copy link
Collaborator

Yeah, I have a tendency to think of typeshed as the main consumer of flake8-pyi, but that's not really the case.

It might be nice to collect the codemods we've applied to typeshed and link them in our CHANGELOG. It could also be nice to update stubgen output to be compliant with all these new lints.

@AlexWaygood
Copy link
Collaborator

Yeah, I have a tendency to think of typeshed as the main consumer of flake8-pyi, but that's not really the case.

It might be nice to collect the codemods we've applied to typeshed and link them in our CHANGELOG. It could also be nice to update stubgen output to be compliant with all these new lints.

Linking to the PRs in the CHANGELOG sounds like a great idea.

We could maybe also have somewhere to collect some of the scripts we've used for codemodding — either a new repo, or a subdirectory in this repo. WDYT?

@hauntsaninja
Copy link
Collaborator

A scripts directory here seems like a decent place, at the very least we're all here now!

@hoefling shared an excellent libcst script in python/typeshed#5564 (comment) that we could add on to.

@Akuli
Copy link
Collaborator

Akuli commented Jan 19, 2022

I don't like the idea of collecting everything into a scripts directory:

  • We don't want to write tests for the scripts, or maintain them in general.
  • We needed to write scripts for typeshed, because there's so many lines of stubs in typeshed. Most other stubs projects don't have anywhere near as much code, and should be fine with manually editing the lines that this plugin points out.
  • The scripts assume the code is blacked, have a few weird corner cases, and aren't exactly clean code. (This applies mostly to my scripts, though.)

I think it would be better to instead improve error messages to say what you should do instead. Some of our error messages already work that way:

Y019 Use "_typeshed.Self" instead of "_S", e.g. "def bad_instance_method(self: Self, arg: bytes) -> Self: ..."

This makes writing a script easy enough to just do yourself if you need one. We could even include one in this repo, and if it's simple enough, it shouldn't be too much work to test and maintain.

Improving stubgen output would also be nice :)

@AlexWaygood
Copy link
Collaborator

I wasn't envisioning writing tests for the scripts. We could have a README file in the subdirectory that has a big "USE WITH CAUTION — UNTESTED" notice in it.

I also didn't mean to suggest that we needed to include every codemodding script we'd ever written in the subdirectory. It could be a selection.

@JelleZijlstra
Copy link
Collaborator Author

Looks good, but I think we should maybe advertise this change more loudly

I added a section listing codes users may not want to enable. Your Y027 can also get a mention there.

I'd be OK with adding cleanup scripts to this repo, but let's do that in a separate PR.

@AlexWaygood
Copy link
Collaborator

I'd be OK with adding cleanup scripts to this repo, but let's do that in a separate PR.

agreed.

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

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.

Revisit disabled-by-default errors Compatibility with flake8 --extend-select
4 participants