-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add an option to require ignore comments have error codes #11633
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 an option to require ignore comments have error codes #11633
Conversation
9ce13fe
to
a99fd9b
Compare
This comment has been minimized.
This comment has been minimized.
The mypy-primer output here is unexpected, right? The new error is off by default so it shouldn't trigger on existing code. |
Indeed, I'm not sure what's causing that. When I run Separately, I have noticed that if I run
Changing the test file seems to get the error to show up, so I'm wondering if there's some caching that I've missed? |
re: primer, I think you might have automatically included it in |
Ahhh, yeah. That was actually intentional and I'd forgotten I'd done it. I'm not sure whether this qualifies for Perhaps it would be better to introduce this flag first and then later move it into |
Yeah, I think we should introduce this first and only later consider moving it into My approximate mental model for
|
Ah, fair. I've personally found that while a lot of things were
Good point, I think it should be easy to add that. |
Also make it non-strict for now. It may become part of --strict in the future, though work may be needed to reduce the number of 'misc' errors before then.
Stylistically matching --warn-unused-ignores' docs.
This comment has been minimized.
This comment has been minimized.
This also includes it in the OPTIONS_AFFECTING_CACHE, which fixes the caching issue I'd been seeing.
"Disallow" didn't feel right here since mypy was still obeying the ignore comment. "Warn" is more precise since this is configuring a new warning to be emitted. This also adjusts the pluralisation in a way I think is better.
I believe I've fixed this in c811aa7. |
I think this would be better enabled via (Some of the existing flags would work better using |
82cb7ab
to
f113c4a
Compare
e082301
to
732c92a
Compare
In preference to adding another specific command line flag.
Users who turn ignore-without-code on are likely at first to encounter cases where their existing ignores are too broad. The previous hint spelling was useful, however could encourage users to just ignore everything which was already ignored rather than actually reviewing the code. This change removes the implication that the correct course is to blindly update the ignore comment while still containing the useful information about what is ignored.
732c92a
to
6f39906
Compare
It's been moved to a pure error code style option.
@JukkaL thanks, I've changed over to this being an error code (still disabled by default). |
I can handle getting this through, since Jukka and Shantanu already commented here without objections to the overall idea. If you fix the conflict, I'll do another round of review, then merge if everything looks good. |
Thanks! I'll fix that conflict now 👍 |
This comment has been minimized.
This comment has been minimized.
Looks like a test needs to be updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor feedback on the docs
Just curious, what happens if you have a |
This appears to have originated in python#12067 which was picked up by 61e9589. TODO: Work out if we should minimise the number of errors that users end up with in this scenario.
Currently nothing specific from this new error code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
One thing I have spotted here is that it's now possible to have several errors from the same problem, as shown by the # flags: --enable-error-code ignore-without-code --warn-unused-ignores
z # type: ignore[ignore-without-code] generates three messages:
While I'm not sure if this is something created by this PR, is this something we want to address here? |
This comment has been minimized.
This comment has been minimized.
I'd rather cover that in a separate PR. Maybe we should suppress the |
This comment has been minimized.
This comment has been minimized.
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
Just want to express my gratitude to @PeterJCLaw for writing and @JelleZijlstra for reviewing this PR. We have been waiting patiently for this function, nice to see it got merged 🎉 Thanks guys! |
Description
Fixes #11509
This introduces a new error code
ignore-without-code
which achieves that.I've gone with naming thisThis is disabled by default but can be controlled via thewarn-
rather thandisallow-
since the ignores are still obeyed, just alerted about (and also as the most closely related options are alsowarn-
style).--enable-error-code
machinery.This new option is aware of
--warn-unused-ignores
(since they're likely to be used together) such that we don't emit warnings about a lack of error codes if the ignore is unused and that option is turned on.One thing I'm not sure about is what should happen for ignore comments that apply to a whole file. I've raised this on that issue, so discussion should probably happen there.Currently the new option obeys that instruction to ignore the file.Test Plan
Some manual testing, plus automated tests added.
Code Review
I'm fairly happy with the code approach here, though I'm not at all sure that I've put the command line option in the right places. I also suspect I've missed some places that would need updating in the docs. Guidance around the proper places to put the docs/CLI arg/etc. is most welcome.(I believe I've now caught everywhere that this needs to go)