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

Add warn_name_override #15187

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

christianbundy
Copy link
Contributor

@christianbundy christianbundy commented May 5, 2023

Fixes #1013

The issue states:

The reason is that a lot of code doesn't define these names consistently, and mypy would generate a ton of useless errors if it insisted on compatibility here.

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:

  • Whether to make this optional (or if it does remain an option, whether it should be enabled by default)
  • Bike-shedding both the name of the option and relevant descriptions
  • Any other changes I'd need to make -- this seems suspiciously simple

@christianbundy christianbundy marked this pull request as ready for review May 5, 2023 04:55
@github-actions

This comment has been minimized.

@gandhis1
Copy link

gandhis1 commented May 5, 2023

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.

@github-actions

This comment has been minimized.

@gandhis1
Copy link

gandhis1 commented May 5, 2023

I quite like this change. This is analogous to the pylint check arguments-renamed: https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/arguments-renamed.html

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 pylint very well might have already addressed them.

@christianbundy
Copy link
Contributor Author

christianbundy commented May 5, 2023

I like that -- would you suggest keeping an option to disable this for teams who don't care, or just enabling it across the board?

Also, it looks like there are tons of failures for this rule in the Mypy repo: do we generally want to try to fix those in the PR that adds the new rule? I want to make sure I'm not making a confusingly large diff.

Found 1039 errors in 31 files (checked 170 source files)

I thought about this a bit more, and my suggestion is:

  1. Merge this as-is or with cosmetic changes (i.e. naming)
  2. In the future, create a PR that enables this in Mypy's self-check and fixes all violations
  3. Further in the future, consider making it enabled by default

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.
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented May 6, 2023

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)

@tmke8
Copy link
Contributor

tmke8 commented May 8, 2023

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 default_enabled=False. Users can then enable it with --enable-error-code warn-name-override or enable_error_code = warn-name-override in the config file.

@hauntsaninja
Copy link
Collaborator

+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).

@christianbundy
Copy link
Contributor Author

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!

@christianbundy christianbundy marked this pull request as draft May 15, 2023 15:52
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.

Check keyword argument compatibility across class hierarchies
4 participants