-
-
Notifications
You must be signed in to change notification settings - Fork 255
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
Initial mypy support #2747
Initial mypy support #2747
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 don't have any experience with MyPy, but it seems like it would be a great addition! However, it would be best if it could also use a generic version of utils/pylint-parser.py
, so it could run only on modified files in a PR, instead of all the files, which would be pretty much unreadable. It would also be nice if there could be another action that could comment the PR with the results, like with Pylint again (maybe in the same comment, to avoid too many comments?)
This is very bad idea,
C.py file
Swapping arguments in C file will automatically show error in A file(if checking only the changed files was done, then basically there would be no point in using mypy)
In the main branch, mypy should always detect 0 errors, so if in any PR it indicates more than 0 it means that the person who created it must correct them and no errors should be hidden from it(in my experience, I know that very rarely is the list of errors greater than 5/10) |
I see, I was saying that it could only log for files that were modified, but run on the whole codebase. It will take a bit of time to fix all types errors, but it's interesting to have the log only on modified files in the PR, don't you think? That's what is currently done with Pylint for example, as otherwise the logs would be thousands of lines long |
Great to see any progress about Code Quality improvement! however there are two issues I found that need to be fix in this PR:
Also, is MyPy our only choice? |
For me this |
I think the sentence I'm bad in English, correct me if I'm wrong. |
I'm agree with this to make everything less ambiguous tho. |
Pylint result on modfied files:
|
Hey! I'm taking this out of the grave, because I have time to work on this again. I've fixed conflicts and let the CI run, but it seems to have failed... Also, it seems that it want to cache something? How will the cache work between CI runs? |
Probably fixed CI by installing manually all typing libraries. |
Seems to have worked, but your update to requirements broke Pytest action. Can you fix it? Thanks! Is it normal that we don't have any errors? When all of it is done, I think we can merge! I'll maybe add a commenter so we can have a quick link to the report, but I'll do it later |
Description
This PR adds initial mypy support.
I only fixed some occurrences of
implictic-optional
errors.For now there is still a lot of disabled checks in
mypy.ini
, that needs to be enabled, but this should be done incrementally in several small PR's.Fixes #2746
First CI run should fail because I commented disabled check in mypy.ini, to see if CI works properly
Type of change
How Has This Been Tested?