-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 warn_name_override
#15187
base: master
Are you sure you want to change the base?
Add warn_name_override
#15187
Conversation
This comment has been minimized.
This comment has been minimized.
It would be helpful to temporarily turn this on by default so that mypy primer could show the impact on a number of publicly accessible code bases. |
This comment has been minimized.
This comment has been minimized.
I quite like this change. This is analogous to the pylint check I would even in be in favor of this being default behavior. Based on what I see in the primer I agree with your assessment that this would cause a number of new failures, but not necessarily an unmanageable amount. And anyone using |
I thought about this a bit more, and my suggestion is:
I'd like for this to be the default, but I also don't want perfect to be the enemy of good, and I don't think I could advocate for a new default that has to be immediately disabled in Mypy's self-check. |
This reverts commit 1153541.
This comment has been minimized.
This comment has been minimized.
Diff from mypy_primer, showing the effect of this PR on open source code: spark (https://github.com/apache/spark) got 1.05x faster (116.3s -> 110.4s)
|
I think the reason that mypy doesn't check the names in overrides was that before Python 3.8, there was no way to specify positional-only arguments, so as a compromise, every argument is considered positional-only. So, I imagine that once mypy stops supporting Python 3.7, this feature would be turned on by default. See also As to the PR itself, isn't it recommended nowadays to add an optional error code instead of a new flag? So, add it here and set |
+1 to what tmke8 mentions about preferring error codes over new flags. I think this is too disruptive to be made the default, even once Python 3.7 is dead. There would need to be a much stronger culture around using positional-only arguments than there is in Python today (and getting there is made difficult by the fact that going from positional-or-keyword to positional-only is a breaking change). |
Sounds good to me, I'll convert this to a draft to keep it from clogging the review queue, and when I have some time to reconfigure it as a new error I'll re-request review. Thanks for the advice! |
Fixes #1013
The issue states:
I've made this optional, but only noticed a few dozens of issues when I ran it against a codebase with hundreds of thousands of lines of code, and one of them was a real problem that could've raised an exception at runtime.
Very open to suggestions, especially: