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

mypy tests: Allow per distribution strictness options #1526

Closed
JelleZijlstra opened this issue Aug 6, 2017 · 13 comments
Closed

mypy tests: Allow per distribution strictness options #1526

JelleZijlstra opened this issue Aug 6, 2017 · 13 comments
Labels
project: infrastructure typeshed build, test, documentation, or distribution related

Comments

@JelleZijlstra
Copy link
Member

We should try turning on the --disallow-incomplete-defs flag from python/mypy#3744. This can catch issues where we forget type annotations for some function arguments in a stub.

@gvanrossum
Copy link
Member

Good idea.

@ilinum
Copy link
Contributor

ilinum commented Aug 10, 2017

I agree :)

There are a lot of errors (little over 5000).
Here is the gist with all the errors it produces.

Not sure if the effort to fix all of them so we can enable the flag is worth it.

@OddBloke
Copy link
Contributor

Considering only the mypy --python-version 3.6 --strict-optional --disallow-incomplete-defs failures, the breakdown between stdlib/third-party is

$ cut -d/ -f1 < out | sort | uniq -c
     77 stdlib
    895 third_party

so enabling this just for the stdlib initially might make addressing them more tractable?

(Full breakdown by module at https://gist.github.com/OddBloke/dbd78409dcd53bdbb6b3b8571bd29720)

@CraftSpider
Copy link
Contributor

I'm interested in tackling at least the stdlib side of this. Would it be better to do this as individual, more well thought-out PRs for each module, or one much larger commit that only does relatively general types

@srittau
Copy link
Collaborator

srittau commented Jan 3, 2020

In general, smaller PRs are easier to review, but too many PRs can also become cumbersome. It might be best to have one PR per package if a package has a few changes needed and a few PRs for packages that only have few changes.

@hauntsaninja
Copy link
Collaborator

Note --disallow-untyped-defs is actually stricter than --disallow-incomplete-defs and should be preferred.

@srittau
Copy link
Collaborator

srittau commented Jun 11, 2020

I am not sure we should enable either. I prefer to have unannotated types over using Any. I also prefer unannotated types over not having definitions or using "incomplete" markers. Enabling these warnings would raise the bar for contributions, with more complex libraries significantly.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jun 11, 2020

I agree on both points! (Although it's reasonable to aspire to turning this on as a lint for stdlib one day, since we're close to completion and hopefully the stdlib isn't changing too drastically).

My point with the above was just that --disallow-untyped-defs is more in line with what I want for identifying stubs that need improvement (in the stub context, it can be a surprise that --disallow-incomplete-defs doesn't complain about definitions that are missing types entirely). If someone is using these flags to improve typeshed, it's a good thing to know.

@srittau
Copy link
Collaborator

srittau commented Jun 11, 2020

Good point about stdlib.

@JukkaL
Copy link
Contributor

JukkaL commented Jun 11, 2020

Once we've migrated to modular typeshed, it should be easy to add support for specifying stricness options for each distribution separately in the metadata file. This wouldn't directly help with the stdlib, since stdlib would be distributed as a single entity.

@srittau srittau added the status: deferred Issue or PR deferred until some precondition is fixed label Sep 17, 2020
@srittau srittau changed the title Perhaps run mypy with --disallow-incomplete-defs mypy tests: Allow per distribution strictness options Sep 17, 2020
@srittau
Copy link
Collaborator

srittau commented Sep 17, 2020

I renamed this ticket to what I believe the consensus of the discussion is. Please revert if you don't agree.

JelleZijlstra pushed a commit that referenced this issue Apr 8, 2021
As requested by #1526.

This addition takes mypy configuration from each distribution metadata file and constructs a single mypy.ini to run with. It assumes there is no mypy.ini but in case we ever need one, it would be simple to add these on top of an existing configuration file.
Might be relevant for #2852

As the issue did not really specify how the configuration would look, I added the following:
- You may add a mypy-tests section to the metadata file.
It looks like this:
[mypy-tests]
[mypy-tests.yaml]
module_name = "yaml"
[mypy-tests.yaml.values]
disallow_incomplete_defs = true
disallow_untyped_defs = true

- module_name can be of the form "a.*" like in mypy.ini.
- You can add several module sections for complex distributions with several modules.
- I added the '--warn-incomplete-stub' option since it is made specifically for typeshed runs. See docs.
@srittau srittau removed the status: deferred Issue or PR deferred until some precondition is fixed label Jun 8, 2021
@srittau
Copy link
Collaborator

srittau commented Jun 8, 2021

I've removed the "deferred" label as this could now be implemented.

@srittau srittau added project: infrastructure typeshed build, test, documentation, or distribution related and removed project: policy Organization of the typeshed project labels Aug 23, 2021
@JelleZijlstra
Copy link
Member Author

Fixed in #5169

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project: infrastructure typeshed build, test, documentation, or distribution related
Projects
None yet
Development

No branches or pull requests

8 participants