-
-
Notifications
You must be signed in to change notification settings - Fork 32k
gh-109413: libregrtest: Add and improve type annotations #109405
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure that the config file without associate script is useful in the PR.
Here is a first review.
It can be used for local testing. You can Without the config file, I don't know how to verify that the changes I'm making in this PR are correct. If I don't know whether the annotations I'm adding are correct or not, then I don't think there's much point in adding them. |
Though I suppose I can keep the config around locally, but not commit it in this PR, if you prefer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please keep self.testsuite_xml: list[str] = []
bug on purpose for now. The code smells, maybe something is wrong here.
Thanks for the review @vstinner! |
@vstinner, this is my attempt to create a PR with only "non-controversial" type hint changes, as you requested in #109382 (review). I'm not fully sure I understand exactly which changes you consider to be controversial, though, so apologies if there are still some changes here that you feel should be deferred to a future PR :-)
(For example: is the addition of a mypy config file in this PR okay? If not, I'm not sure how to test locally the changes I'm making here.)
This PR gets us down to 20 mypy errors if you
cd
intoLib/test
and then runmypy --config-file libregrtest/mypy.ini
.