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

Basic type-checking with mypy and pyright #2102

Merged
merged 4 commits into from
Mar 14, 2024

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Aug 11, 2023

This is the PR that finally makes basic type-checking validation of public methods possible, easing the addition of 3.7+ type annotations.

In its current state, a lot of checks are disabled, and some areas of code are completely ignored. But a handful of critical issues are now checked by GitHub actions and will help prevent some regressions.
A few typing fixes have already been provided where I considered they would be worth over disabling globally.

Once this PR is merged, I can in parallel start:

NoTranslateMap = {}
for v in NoTranslateTypes:
NoTranslateMap[v] = None
NoTranslateMap = set(NoTranslateTypes)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this intended to be public API? If not I'd rename it to NoTranslateSet

@@ -52,7 +56,8 @@

# A dictionary of ITypeLibrary objects for demand generation explicitly handed to us
# Keyed by usual clsid, lcid, major, minor
demandGeneratedTypeLibraries = {}
# Typing as Any because PyITypeLib is not exposed
demandGeneratedTypeLibraries: dict[tuple[str, int, int, int], Any] = {}
Copy link
Collaborator Author

@Avasam Avasam Aug 11, 2023

Choose a reason for hiding this comment

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

Any should be used sparingly, it's an escape hatch. For instance: When it's impossible to represent a type accurately using the current type system.

In this case, we have no way to represent this type though python code (for now). And I need to pass something to the generic type parameter.

Avasam added a commit to Avasam/pywin32 that referenced this pull request Sep 21, 2023
@Avasam
Copy link
Collaborator Author

Avasam commented Nov 1, 2023

I'm not sure I understand the test failure here. Maybe an infinite loop/wait that causes it to run for hours until it times out?
Merging other related PRs first will lead to less changes here and make it easier for me to figure out where the issue comes from.

Edit: This has now been resolved

mypy.ini Outdated
; Most of win32com re-exports win32comext
; Test is a local untyped module in win32comext.axdebug
; Not sure where the rest of modules/packages come from
[mypy-verstamp,win32com.*,Test,CallTipWindow,dde,pywin32_system32,]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any idea where CallTipWindow, dde and pywin32_system32 come from?

Copy link
Owner

Choose a reason for hiding this comment

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

dde is a module - https://github.com/mhammond/pywin32/blob/main/setup.py#L2025
pywin32_system32 is a hack setup.py does to put the DLLs in a place where they aren't technically a package, eg, https://github.com/mhammond/pywin32/blob/main/setup.py#L337-L339
IIRC, CallTipWindow was a part of idle which we hackily reused -

def _make_tk_calltip_window(self):
import CallTipWindow
return CallTipWindow.CallTip(self.text)
- I doubt it works at all any more.

Copy link
Collaborator Author

@Avasam Avasam Nov 2, 2023

Choose a reason for hiding this comment

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

I went on a deep dive about CallTipWindow yesterday. It comes from pre-Python 3.6 idlelib code. A lot of other modules have been renamed in Python 3.5. Some even have been merged into other modules way before that. Relevant PRs I found:

So yeah, I too doubt this works anymore. Not anything for this PR to do. Not sure what you wanna do with that either. But for now I can at least document why CallTipWindow is not a found module.

@Avasam Avasam marked this pull request as ready for review February 27, 2024 21:44
@Avasam
Copy link
Collaborator Author

Avasam commented Feb 27, 2024

@mhammond All issues have finally been resolved! If you want, you can merge #2175 first. Otherwise merging this will close #2175 too

mhammond pushed a commit that referenced this pull request Feb 28, 2024
@mhammond
Copy link
Owner

Thanks, but I'm seeing many annotations in the github "changed files" view which shouldn't be there, near the end of the PR:

image

I think we need to get rid of these before it can merge.

@mhammond
Copy link
Owner

Re "Solve some pyright warnings in changed files" - does this imply that in the future, when other changed files are touched, these same warnings might appear in those files?

@Avasam
Copy link
Collaborator Author

Avasam commented Feb 28, 2024

Re "Solve some pyright warnings in changed files" - does this imply that in the future, when other changed files are touched, these same warnings might appear in those files?

If a warning is present in a file that has been modified, yes.
Not that these are warnings, not error. They are meant to say "this needs to be looked at, or fixed eventually (but won't block the PR)". There is currently 471 errors that I have downgraded to warnings in pyrightconfig.json because otherwise the CI wouldn't pass in its current state (and the goal of this PR is to do the minimum so that public type-annotations can be type-checked), but they are valid pyright reports.

As for the fact that they show up as "GitHub annotations", I think I can disable that entirely from what I'm reading. https://github.com/actions/toolkit/blob/master/docs/commands.md#problem-matchers

Edit: Didn't seem to work. If these annoy you, I can turn them to "info" in pyright configs.
Edit 2: Figured it out, no more annotation / comment pollution in files tab.

pyrightconfig.json Outdated Show resolved Hide resolved
@Avasam
Copy link
Collaborator Author

Avasam commented Feb 29, 2024

As a sidenote before this comes up: Test dependencies should probably be pinned. And even better: written to a requirements file so they can be deduplicated, easy to install, and their download cached by the setup-python action. But no dependency is pinned currently in pywin32. So that's a change I'd propose in a separate follow-up PR.

@Avasam
Copy link
Collaborator Author

Avasam commented Mar 13, 2024

@mhammond I think all of your latest concerns have been addressed here.

Btw all of my currently opened PRs are ready for review / hoping for a merge. You can use this query to ignore adodbapi-related ones: https://github.com/mhammond/pywin32/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+author%3AAvasam+NOT+adodbapi

Copy link
Owner

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

Thanks!

@mhammond mhammond merged commit e2906a8 into mhammond:main Mar 14, 2024
27 checks passed
@Avasam Avasam deleted the basic-type-checking branch March 14, 2024 14:27
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.

3 participants