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 option to selectively disable --disallow-untyped-calls #15845

Merged
merged 4 commits into from Aug 13, 2023

Conversation

ilevkivskyi
Copy link
Member

Fixes #10757

It is surprisingly one of the most upvoted issues. Also it looks quite easy to implement, so why not. Note I also try to improve docs for per-module logic for disallow_untyped_calls, as there is currently some confusion.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks, this seems useful! Not a full review.


.. code-block:: python

# mypy --disallow-untyped-calls --untyped-call-exception=foo --untyped-call-exception=bar.A
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe also use a real third-party package in the example, such as numpy?

assert object_type is not None
fullname = self.method_fullname(object_type, member)
if not fullname or not any(
fullname.startswith(p) for p in self.chk.options.untyped_call_exception
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make sure the exception re doesn't apply to retry, i.e. the startswith operation should probably be at module name component level.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

disallow_untyped_calls = True
untyped_call_exception = some.library

.. confval:: untyped_call_exception
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems useful, but I'm not sure about the name of the option. Maybe we can come up with a better name? I don't any suggestions right now, but I'll think about this later.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also struggled some time with this. I would propose to not spend much time on this. People who want this will probably search for "mypy untyped call" or similar, so including these words in the option name should be good enough. So if you have a better idea, then I will be happy to update, otherwise let's just move on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fwiw I'd go with untyped_call_allowlist. Clearly conveys the type as well ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is what I considered at first, but then how wold we call corresponding command line option? --untyped-call-allowlist? All other options like --always-true, --always-false, --enable-error-code, --disable-error-code etc, are singular and should be repeated. So I would say we should try to follow this pattern. Finally, from reading comments in the issue, it looks like most people will need just 1 or maybe 2 exceptions, not more.

:type: comma-separated list of strings

Selectively excludes functions and methods defined in specific packages,
modules, and classes from action of :confval:`disallow_untyped_calls`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mention that this also applies to submodules of packages (i.e. everything inside that prefix).

This flag allows to selectively disable :option:`--disallow-untyped-calls`
for functions and methods defined in specific packages, modules, or classes.
Note that each exception entry acts as a prefix. For example (assuming there
are no type annotations for ``numpy`` available):
Copy link
Member

Choose a reason for hiding this comment

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

Numpy's probably not a great example here, as they ship with a py.typed file these days and have pretty complete type annotations. Maybe one of matplotlib, requests or TensorFlow might be a better example? Third-party stubs exist for all three packages, but none of them ships with a py.typed file

Copy link
Contributor

Choose a reason for hiding this comment

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

Any example of a popular library might become a bad example over time, maybe limit ourselves to something generic like third_party_lib?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, let's use some generic name.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@ilevkivskyi
Copy link
Member Author

IIUC the only remaining question now is the name of the new flag. I propose that instead of keeping this open indefinitely, merge this as is, and then think about it until the next release. It will be easy to rename it if a better idea appears. If there are no objections, I am going to merge this tomorrow or Monday.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks, this is a nice feature. Agreed we can merge. How do you feel about just --untyped-call-allow? I like using "allow" because it's a verb we use in a lot of places and because exception makes me think of the language feature

@ilevkivskyi
Copy link
Member Author

Yes, -exception can make one think this is something about exceptions. But also then --untyped-call-allow would be too similar to --allow-untyped-calls (which is also a valid flag). I think it may be better to use --untyped-calls-exclude because:

  • It is also a verb an indeed we mostly use verbs for many flags (especially for strictness flags)
  • It doesn't have the exception connotation
  • It is sufficiently different from --exclude flag (and they are kind of similar btw, exclude also expects list of patterns)
  • It still seems to clearly indicate what it does: exclude certain functions from untyped calls check.

@github-actions
Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@hauntsaninja
Copy link
Collaborator

Great, I like it!

@ilevkivskyi ilevkivskyi merged commit 117b914 into python:master Aug 13, 2023
19 checks passed
@ilevkivskyi ilevkivskyi deleted the untyped-allowlist branch August 13, 2023 20:20
@WilliamKozickiGF
Copy link

Hey, I just want to say, god bless you guys for adding this feature. I started using mypy last week and have thought that there has to be a way to do what untyped_calls_exclude does. It's hard to believe that this was only added this recently.

@johnthagen
Copy link
Contributor

johnthagen commented Oct 30, 2023

For those looking for a pyproject.toml config example for this:

[tool.mypy]
strict = true
untyped_calls_exclude = [
    "skimage"
]

This ignores all errors from missing type annotations in the skimage third party package when imported and used in user code.

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.

disallow_untyped_calls in module does not override global section
7 participants