Skip to content
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

Bump mypy to latest version #5767

Merged
merged 19 commits into from
Jul 15, 2022
Merged

Bump mypy to latest version #5767

merged 19 commits into from
Jul 15, 2022

Conversation

maffoo
Copy link
Contributor

@maffoo maffoo commented Jul 14, 2022

Prompted by discussion on #5486; this turned out to be not as difficult as I thought somewhat difficult.

@maffoo maffoo requested review from a team, vtomole and cduck as code owners July 14, 2022 21:16
@CirqBot CirqBot added the Size: XS <10 lines changed label Jul 14, 2022
@CirqBot CirqBot added the size: S 10< lines changed <50 label Jul 14, 2022
mpharrigan
mpharrigan previously approved these changes Jul 15, 2022
Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type check test is failing

@mpharrigan mpharrigan dismissed their stale review July 15, 2022 02:08

type check test is failing as it turns out

@maffoo
Copy link
Contributor Author

maffoo commented Jul 15, 2022

type check test is failing

Yeah, there seem to be differences between running mypy in my local environment and in CI, so I missed these failures locally. I'll take a look to see how tricky these errors are to fix.

@@ -119,7 +119,7 @@ def inner_product_of_state_and_x(self, x: int) -> Union[float, complex]:
* 2 ** (-sum(self.v) / 2)
* 1j**mu
* (-1) ** sum(self.v & u & self.s)
* np.all(self.v | (u == self.s))
* bool(np.all(self.v | (u == self.s)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use (...).item() to turn numpy thing into python thing?

@mpharrigan
Copy link
Collaborator

there seem to be differences between running mypy in my local environment and in CI

This sounds worrying. Do you have any idea why?

@dabacon
Copy link
Collaborator

dabacon commented Jul 15, 2022

rences between running mypy in my local environment and in CI, so I missed these failures locally. I'll take a look to see how

Yeah I had this too, and was a very recent install. Maybe because our python versions are higher?

@maffoo
Copy link
Contributor Author

maffoo commented Jul 15, 2022

@dabacon, the problem in my case was an older version of numpy without type annotations. I think it's working now.

@maffoo maffoo requested a review from mpharrigan July 15, 2022 19:40
@maffoo
Copy link
Contributor Author

maffoo commented Jul 15, 2022

I think I've got everything now; PTAL @mpharrigan, @dabacon.

I found a smattering of random lint errors that were revealed when touching various files. FWIW I think we should change CI to lint everything, because the current incremental linting means there can be lint errors hiding in a file that have to be handled by the next person to touch the file for unrelated reasons.

Copy link
Collaborator

@dstrain115 dstrain115 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look like reasonable changes to me.

@maffoo maffoo added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 15, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 15, 2022
@CirqBot
Copy link
Collaborator

CirqBot commented Jul 15, 2022

Automerge cancelled: A required status check is not present.

Missing statuses: ['Build docs', 'Build protos', 'Coverage check', 'Doc test', 'Format check', 'Misc check', 'Notebook formatting', 'Pytest MacOS (3.7)', 'Pytest MacOS (3.8)', 'Pytest MacOS (3.9)', 'Pytest Ubuntu (3.7)', 'Pytest Ubuntu (3.8)', 'Pytest Ubuntu (3.9)', 'Pytest Windows (3.7)', 'Pytest Windows (3.8)', 'Pytest Windows (3.9)', 'Type check', 'Typescript lint check', 'Typescript tests', 'Typescript tests coverage']

@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jul 15, 2022
@maffoo maffoo merged commit 3386c09 into master Jul 15, 2022
@maffoo maffoo deleted the u/maffoo/mypy-bump branch July 15, 2022 22:49
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: S 10< lines changed <50 Size: XS <10 lines changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants