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

Allow PEP 561 packages to specify mypy settings #5260

Closed
emmatyping opened this issue Jun 21, 2018 · 14 comments
Closed

Allow PEP 561 packages to specify mypy settings #5260

emmatyping opened this issue Jun 21, 2018 · 14 comments

Comments

@emmatyping
Copy link
Collaborator

emmatyping commented Jun 21, 2018

[This is a summary of a discussion on the typing gitter, if I missed anything please do mention it]
With the expectation of many new packages being created conforming to PEP 561, @ilevkivskyi suggested that package authors should be able to set strictness settings on their code.

Ivan's original suggestion is to allow setting some flags like --ignore-missing-imports, --no-strict-optional in the source file(s) themselves.

I suggested per-package, as defining and implementing a new format seemed like a lot of work. Also, I generally am against metadata in source code, as I find it noisy.

@Michael0x2a wanted the ability to override this.

Michael then suggested we mandate PEP 561 packages provide a config file and mypy can read per-module config options from said file.

Ivan suggested amending the suggestion to mypy will allow per-module config files in PEP 561 packages.

I concurred with Ivan.

tl;dr

I suggest that mypy read per-module config file settings, if a config file is found.

@Michael0x2a
Copy link
Collaborator

FWIW, I agree "allow" is better then "mandate". I used the word "mandate" mostly because it was the first word that came to mind, not due to any actual technical reason.

One way to allow overriding might be to let the user's mypy.ini config file override the package's config -- so if I added a [somelibrary.submodule] section to my ini file, those rules would override anything the library defined there.

That said, I'm not sure how easy it would be to implement the above. If it turns out to be tricky/have lots of edge cases, I think it'd be fine to make overriding low priority/just not implement it at all.

@JelleZijlstra
Copy link
Member

In typeshed, we've also encountered this issue, but it's not been a big deal because we run typeshed's tests under fairly strict mypy options, and I believe mypy has some special casing to suppress errors from typeshed.

Perhaps we can do something similar for PEP 561 packages: recommend stub file maintainers to test their stubs with the strictest possible options for mypy, and within mypy suppress some errors from stub packages. The latter makes sense to me because users don't have direct control over the content of stub packages, so it's not useful for them to see errors that show up within PEP 561 packages. If we do this, we may not need special handling of config files within PEP 561 packages.

@gvanrossum
Copy link
Member

But PEP 561 also supports packages with inline annotations. For those a local mypy.ini seems a good idea.

@JukkaL
Copy link
Collaborator

JukkaL commented Jun 22, 2018

Another way would be to ignore all errors from PEP 561 packages by default. New mypy versions can introduce stricter checks (fixes to false negatives etc.) that can't be disabled and thus PEP 561 inline annotations could easily start generating errors after updating mypy. This could become a major problem for projects that rely on many PEP 561 packages with inline annotations -- they could only move to a new mypy version after all their dependencies have been migrated.

@ilevkivskyi
Copy link
Member

Another way would be to ignore all errors from PEP 561 packages by default.

Yes, this is a good idea. Errors from installed packages can be a major block for PEP 561 adoption so we should minimise the risk of this as much as possible.

@gvanrossum
Copy link
Member

@ethanhs Maybe we should put this as a recommendation in PEP 561 itself?

OTOH I think it's too soon to know whether we will see widespread adoption of packages with inline annotations -- personally my money is on stub packages.

[I know Ethan is traveling so it may be a while before we get a response.]

@emmatyping
Copy link
Collaborator Author

In my first implementation I actually did ignore errors in PEP 561 packages. However, it had 2 side effects that were not savory to me. One was that it would ignore typeshed on Windows (which I now realize should be pretty easy to work around). The other more concerning one is that errors in stubs can cause propogations of issues into user code, so silencing errors in PEP 561 packages, just like silencing errors in typeshed, seems potentially dangerous. Furthermore it would be harder for maintainers to PEP 561 packages, as they won't get errors if they install them.

I'd say mypy should either add a flag to silence errors from PEP 561 packages, maybe even allowing listing particular packages, or read a mypy config file from the package.

@JukkaL
Copy link
Collaborator

JukkaL commented Jun 25, 2018

The other more concerning one is that errors in stubs can cause propogations of issues into user code, so silencing errors in PEP 561 packages, just like silencing errors in typeshed, seems potentially dangerous.

Yes, this is a concern, but I was primarily talking about errors in packages with inline annotations. Unexpected mypy errors could be a much bigger issue that what you describe, especially for projects that aren't close to precisely annotated. For example, Any types can generate false negatives that hide bad annotations, and mypy and typeshed upgrades can improve precision in ways that turn those false negatives into actual errors.

From our experience at Dropbox we know that a few weeks of mypy commits will cause from a few to hundreds of new mypy errors in internal Dropbox codebases that we need to fix or work around. If this can be generalized into other large Python projects, other packages with inline annotations would likely start generating errors after a few mypy releases at least. (We have it worse at Dropbox though, since we often run on mypy master with issues that will be fixed before a release.)

Furthermore it would be harder for maintainers to PEP 561 packages, as they won't get errors if they install them.

I don't think that this is a significant concern. Maintainers would just need to run mypy so that it targets sources in their repository instead of an installed package. Sources given explicitly should take precedence over installed packages, right?

@JelleZijlstra
Copy link
Member

I just ran into this sort of issue with a real package. The async_timeout package has py.typed, but it has some errors under --no-implicit-optional (https://github.com/aio-libs/async-timeout/blob/master/async_timeout/__init__.py#L28) and --disallow-any-generics (disallow_any_generics). But I run mypy with both of these options enabled, so I had to manually silence errors from async_timeout in my mypy.ini.

My point is that this is a real-world issue that we should try to fix soon :)

@emmatyping
Copy link
Collaborator Author

Would it be crazy to do both? We could read per-package mypy.ini configs and add a flag for silencing errors in packages.

That way there is greater safety by default, and fewer implicitly created Any types, but if a project upgrades a dependency and finds it has issues with the latest mypy, it can silence them?

@JelleZijlstra
Copy link
Member

That should work, yes, but it makes things more complicated (both for package authors and for us as mypy maintainers, and also for mypy users who have to think about yet another mypy flag).

As a user, I would prefer to never see errors from inside PEP 561 packages or typeshed. There's nothing I can do about them, so they're not useful for me.

@emmatyping
Copy link
Collaborator Author

Yeah, I suppose we can silence errors. I'm not too keen, but I think it is probably the simplest solution.

@gvanrossum
Copy link
Member

I propose to silence them by default but add a flag to show errors in installed packages. And maybe typeshed should be handled the same?

@emmatyping
Copy link
Collaborator Author

I agree a flag to turn the silencing off would be nice. I've added silencing typeshed and --no-silence-site-packages in #5303. I am open for better name suggestions :)

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

No branches or pull requests

6 participants