mhammond / pywin32

Python for Windows (pywin32) Extensions
4.92k stars 786 forks source link

Basic type-checking with mypy and pyright #2102

Closed Avasam closed 4 months ago

Avasam commented 11 months ago

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:

Avasam commented 8 months ago

~~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

Avasam commented 4 months ago

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

mhammond commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

Avasam commented 4 months ago

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 commented 4 months ago

@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