Conversation
|
Opened #1789 to track the growing complaints with e.g. |
mhucka
left a comment
There was a problem hiding this comment.
This looks overall okay, but it seems to mix multiple kinds of changes into one PR:
- Dependency version updates in the various requirements files
- Addition of type annotations
- In the case of
qualtran/serialization/sympy_to_proto.py, addition of imports
It's not immediately clear whether these are actually related and need to be in the same PR. If they do need to be in the same CL, could the reasons be explained in the PR description?
Conversely, if they don't need to be in the same CL, I would recommend following the principle of small CLs (https://google.github.io/eng-practices/review/developer/small-cls.html) and splitting this up into separate PRs.
(I hope this doesn't come across as too critical. Just trying to do a good review.)
|
should have noted that those changes are in response to new mypy and pylint failures that only occur with the updated dependency versions. will update pr description |
pip-compile.sympy_to_proto.py, so I added the imports.NDArary[cirq.Qid]and friends. Added# type: ignoreand opened NDArray[MyObject] type annotations are not allowed #1789 to track longer-term fix