-
-
Couldn't load subscription status.
- Fork 3k
Support error codes from plugins in options #19719
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
Conversation
17b8cac to
3b08bd3
Compare
This comment has been minimized.
This comment has been minimized.
dacdf4b to
c056317
Compare
Mypy has options for enabling or disabling specific error codes. These work fine, except that it is not possible to enable or disable error codes from plugins, only mypy's original error codes. The crux of the issue is that mypy validates and rejects unknown error codes passed in the options before it loads plugins and learns about the any error codes that might get registered. There are many ways to solve this. This commit tries to find a pragmatic solution where the relevant options parsing is deferred until after plugin loading. Error code validation in the config parser, where plugins are not loaded yet, is also skipped entirely, since the error code options are re-validated later anyway. This means that this commit introduces a small observable change in behavior when running with invalid error codes specified, as shown in the test test_config_file_error_codes_invalid. This fixes python#12987.
c056317 to
3ebd792
Compare
This comment has been minimized.
This comment has been minimized.
mypy/main.py
Outdated
| # have loaded. | ||
| options.process_error_codes(error_callback=parser.error) | ||
|
|
||
| options._on_plugins_loaded = on_plugins_loaded |
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.
I don't like this pattern (callback that stores another callback). Instead you should simply use another way to create a blocker error, e.g. use raise CompileError(...) as a error_callback to process_error_codes() after the plugins are loaded.
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.
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.
Nice! I added you as a collaborator on the fork, feel free to modify as you see fit!
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-- committed callback refactor
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.
@ilevkivskyi raising CompileError rather than calling parser.error() has a subtly different result, since only the latter will output CLI usage with the error. Are you sure about this change?
Test expectations fail as follows:
Expected:
usage: mypy [-h] [-v] [-V] [more options; see below] (diff)
[-m MODULE] [-p PACKAGE] [-c PROGRAM_TEXT] [files ...] (diff)
mypy: error: Invalid error code(s): YOLO (diff)
== Return code: 2
Actual:
Invalid error code(s): YOLO (diff)
== Return code: 2
Personally, I think parser.error() is the best for consistency with other parsing error cases. I wouldn't expect a different behavior only because a check needs to be deferred.
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.
It is fine (and probably even good) to not have the usage: ... part in this error. But I would add the mypy: error: prefix.
But also what you did is not what I wanted, the whole "double-callback" logic is pointless, you can simply call options.process_error_codes() directly in build.py.
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Mypy has options for enabling or disabling specific error codes. These
work fine, except that it is not possible to enable or disable error
codes from plugins, only mypy's original error codes.
The crux of the issue is that mypy validates and rejects unknown error
codes passed in the options before it loads plugins and learns about the
any error codes that might get registered.
There are many ways to solve this. This commit tries to find a pragmatic
solution where the relevant options parsing is deferred until after
plugin loading. Error code validation in the config parser, where
plugins are not loaded yet, is also skipped entirely, since the error
code options are re-validated later anyway. This means that this commit
introduces a small observable change in behavior when running with
invalid error codes specified, as shown in the test
test_config_file_error_codes_invalid.
This fixes #12987.