Skip to content

Conversation

@AlexWaygood
Copy link
Member

No description provided.

@github-actions

This comment has been minimized.

@Akuli
Copy link
Collaborator

Akuli commented Dec 1, 2021

What does mypy_primer say if you do this to other similar functions too, e.g. add, sub, mul, matmul? I imagine mypy_primer might give a false sense of correctness, because operator.mod is not used very often, and seeing the output for more similar functions would show whether this is a good idea in general.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Dec 1, 2021

What does mypy_primer say if you do this to other similar functions too, e.g. add, sub, mul, matmul?

The output from primer in #6448 shows that we get large numbers of false positives if we make e.g. sub have a different signature to add (see e.g. the rotki errors in the diff), because people often use functions in the operator module in highly dynamic ways, such that mypy can't be sure which function is going to be called. I don't think mod has the same problem, though, as there's no "opposite" function to __mod__ that's the same as __sub__ with respect to __add__.

However, I need to account for __rmod__ for this to be merged...

@AlexWaygood AlexWaygood marked this pull request as draft December 1, 2021 10:35
@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2021

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood AlexWaygood marked this pull request as ready for review December 1, 2021 11:50
@Akuli
Copy link
Collaborator

Akuli commented Dec 1, 2021

I still find it weird that mod is very different from add and sub. I will let other maintainers decide what to do with this.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Dec 1, 2021

I still find it weird that mod is very different from add and sub. I will let other maintainers decide what to do with this.

My thinking is that sub and add would probably benefit from similar treatment to that which the binary-operator comparison functions received in #6462. I can add that to this PR, if you like?

They would still be treated differently to mod, but less differently.

@AlexWaygood AlexWaygood closed this Dec 6, 2021
@AlexWaygood AlexWaygood deleted the patch-2 branch December 6, 2021 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants