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

Implement flag to allow typechecking of untyped modules #17712

Merged
merged 8 commits into from
Dec 4, 2024

Conversation

DeinAlptraum
Copy link
Contributor

@DeinAlptraum DeinAlptraum commented Aug 27, 2024

Add a flag and config ini options "follow_untyped_imports". Setting it to True instructs mypy to typecheck also modules that do not have stubs or a py.typed marker.

Fixes #8545

This comment has been minimized.

This comment has been minimized.

@DeinAlptraum
Copy link
Contributor Author

@hauntsaninja can you tell me by any chance if there's anything I'm missing/I should do to get a review here?

@DeinAlptraum
Copy link
Contributor Author

Ping. Can someone review this? @JelleZijlstra can you help me with this?

@DeinAlptraum
Copy link
Contributor Author

Ping. @hauntsaninja @JelleZijlstra can you tell me what I need to do to get a review here?

@DeinAlptraum
Copy link
Contributor Author

Ping @hauntsaninja @JelleZijlstra @JukkaL

@JelleZijlstra
Copy link
Member

Mypy development is unfortunately under-resourced and personally I don't have much time available to review PRs. I'd advise you to wait and hopefully a committer will come along and review this.

@Xiddoc
Copy link

Xiddoc commented Nov 22, 2024

I feel like this feature is crucial and affects way too many people for it to be ignored. Using type: ignore or manually adding a py.typed file to every used third-party module is incredibly impractical.

Could this at least get a CR, if improvements are needed then the community can follow up on issues for the PR. ❤️
@JukkaL @hauntsaninja

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 22, 2024

I can try to review this -- possibly next week. It would help if the merge conflict was fixed.

@DeinAlptraum
Copy link
Contributor Author

Thanks for the comment, didn't notice the merge conflict. I wish there were notifications for this... anyway, fixed.

This comment has been minimized.

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! I think the per-module handling isn't quite right, in particular, I'm not sure it handles globs correctly. I'd also vote for a slightly more specific name, maybe something like --follow-untyped-imports. This matches the untyped-import error code the current implementation will silence.

@DeinAlptraum
Copy link
Contributor Author

@hauntsaninja thanks for the feedback! You're right, this wasn't handling globs properly. In the process of fixing that, I also noticed that my solution was more complicated than necessary and simplified it a bit...

Regarding the name: it seems there was quite a lot of discussion about how the config option should be called on the original issue, so I wasn't sure what to pick. Yours definitely sounds better though, so changed it to that for now.

This comment has been minimized.

mypy/modulefinder.py Outdated Show resolved Hide resolved

This comment has been minimized.

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 looks good. I'll merge in a few days if no one else has comments. Are you up for adding docs for this flag as well?

@DeinAlptraum
Copy link
Contributor Author

@hauntsaninja I added it to the docs where it felt sensible to me. Do you think it should be mentioned in any other places?

Copy link
Contributor

github-actions bot commented Dec 2, 2024

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

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.

Looks excellent, thank you for this feature!

@hauntsaninja hauntsaninja merged commit 242873a into python:master Dec 4, 2024
20 checks passed
@DeinAlptraum DeinAlptraum deleted the check-untyped branch December 4, 2024 07:16
@cdce8p
Copy link
Collaborator

cdce8p commented Dec 5, 2024

I tried this flag for Home Assistant today. Unfortunately, mypy couldn't even do a full run and failed instantly. Haven't narrowed it down completely yet, just a first reproducer (tested with Python 3.13):

# test.py
import aprslib.base91
reveal_type(1)
# mypy_config.ini
[mypy]
follow_imports = normal
follow_untyped_imports = true
pip install aprslib==0.7.2
mypy --config-file mypy_config.ini test.py
/.../venv/lib/python3.13/site-packages/aprslib/parsing/common.py:218: SyntaxWarning: invalid escape sequence '\!'
  match = re.findall("^(.*)\!([\x21-\x7b])([\x20-\x7b]{2})\!(.*?)$", body)
venv/lib/python3.13/site-packages/aprslib/parsing/__init__.py: error:
    Source file found twice under different module names: "aprslib.parsing" and "aprslib.parsing.__init__"
Found 1 error in 1 file (errors prevented further checking)

The SyntaxWarning is an unrelated issue.

Will open a new issue, once I've narrowed it down a bit more.

@DeinAlptraum
Copy link
Contributor Author

@cdce8p thanks for reporting, but that seems unrelated to this change.
I just cloned the aprs-python repo and ran the latest mypy release (i.e. before I added this flag) in the root folder of the project and I get the same error. Couldn't really tell you what that means though, since I'm not super familiar with the inner workings of mypy

@cdce8p
Copy link
Collaborator

cdce8p commented Dec 5, 2024

@cdce8p thanks for reporting, but that seems unrelated to this change. I just cloned the aprs-python repo and ran the latest mypy release (i.e. before I added this flag) in the root folder of the project and I get the same error.

Indeed thanks for pointing it out! Thinking out loud for a moment: Would it make sense to add an explicit warning to the docs or am I just the only one who would run into it? With follow_untyped_imports = true it's much more likely that code will be checked which isn't designed for type checking so errors might occur more frequently.

@hauntsaninja
Copy link
Collaborator

Possibly same thing as #9716 ?

@hauntsaninja
Copy link
Collaborator

@hauntsaninja
Copy link
Collaborator

We can add a warning to the docs, done so here: #18249

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

Successfully merging this pull request may close these issues.

Allow/disallow pep561 packages
6 participants