-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add CI checks for mypy and pylint #55
base: main
Are you sure you want to change the base?
Conversation
dstrain115
commented
Jul 7, 2022
•
edited
Loading
edited
- Configuration copied from cirq.
- Adds CI tests for type checking (mypy) and linting (pylint)
- mypy is currently information, since we have a lot of typing errors I need to sift through first.
- Configuration copied from cirq.
FWIW pytype has a lot less problems than mypy on the quantum chess codebase. |
Cirq and other codebases we use all use mypy, so I would like to stay consistent. |
dev_tools/mypy.ini
Outdated
ignore_missing_imports = true | ||
|
||
# 3rd-party libs for which we don't have stubs | ||
[mypy-apiclient.*,freezegun.*,matplotlib.*,mpl_toolkits,multiprocessing.dummy,oauth2client.*,pandas.*,proto.*,pytest.*,scipy.*,sortedcontainers.*,setuptools.*,pylatex.*,networkx.*,qiskit.*,pypandoc.*,ply.*,_pytest.*,google.api.*,google.api_core.*,grpc.*,google.auth.*,google.oauth2.*,google.protobuf.text_format.*,quimb.*,pyquil.*,google.cloud.*,filelock.*,codeowners.*,tqdm.*,importlib_metadata.*,google.colab.*,IPython.*,astroid.*,pylint.*,cirq.*,flask.*,numpy.*,cirq_google.*] |
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.
Wow, do you really have to maintain a list of transitive dependencies to use mypy? I think this will be a problem (if the check is enforced). The set of dependencies can change at any time since the package manager generally downloads the latest versions modulo constraints. So this will lead to CI errors on pull requests unrelated to the actual changes whenever a new transitive dependency appears.
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.
It's not transitive dependencies, it's only direct dependencies. CI errors for this will only be if a new dependency appears that was introduced in the actual PR.
Hopefully I can figure out how to get stubs for some of the libraries as well.
Let me see if I can clean this up.
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.
What do you mean it's not transitive dependencies? For example freezegun
, protobuf
, or flask
are not referenced by the Unitary codebase.
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.
Those were all from cirq, where I copied the continuous integration test from. So, there were a lot of extra unneeded dependencies in there. If you refresh to the latest commit, I have pruned it way down (there may still be a few extra unneeded ones, but I got rid of a bunch of them).
mypy | ||
pylint | ||
pytest | ||
pytest-cov |
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.
Why install coverage tools?
@@ -0,0 +1,73 @@ | |||
[MASTER] | |||
load-plugins=pylint.extensions.docstyle,pylint.extensions.docparams,pylint_copyright_checker | |||
max-line-length=100 |
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.
88 to agree with black?
[mypy] | ||
show_error_codes = true | ||
|
||
[mypy-__main__] |
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.
What does this mean?
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'm not sure how useful I'll be here. I can work my way through python, but I'm no super versed in all of the tooling.