Skip to content

Support silencing only some error codes #8102

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

Closed
wants to merge 4 commits into from

Conversation

ilevkivskyi
Copy link
Member

This PR adds support for silencing particular error codes either on command line, in config file or in inline mypy options. The implementation is mostly straightforward, here are however two important comments:

  • I initially wanted to also add an option to opt-in selected error codes, but this turns out to be much harder, because there are many call sites scattered in the code that check for whether a given flag is enabled. We can later migrate some of them to error codes. Also in my experience people often ask about opting out for a specific error code rather than opting in.
  • If there are two sections in config file that match a given module, the ignore lists are not merged, the more narrow match acts as an override. This is similar to how other options work (like --always-true and --always-false), I am not sure this is the most obvious behavior, but it looks like it is most flexible. We can reconsider this later if people will find it confusing.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this will be a useful feature, especially if/when we add more error codes.

My biggest feedback is that I think that we should merge ignored error codes (see my comment below for why I think so).

.. option:: --ignore-error-codes CODES

This flag makes mypy ignore all errors with given error codes. Flag accepts
error codes as a comma separated list (there should be no spaces after commas).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better for usability if spaces around commas were ignored. Error codes can't have spaces.

Grammar nit: "comma separated" -> "comma-separated" (we use the latter spelling elsewhere)

def __init__(self, attr: str, value: object) -> None:
setattr(self, attr, value)

magic_builtin # error: Name "magic_builtin" is not defined
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments make this example look kind of busy, since some of the messages are long. Also, it would be nice to include the error codes in the message, as now it's unclear where the error codes come from (--show-error-codes is introduced only later).

Here's a suggested change:

  • Remove the comments from the example.
  • Include full output from a mypy run, without --ignore-error-codes flag, but with --show-error-codes.
  • Show output from another mypy run, now with the the various error codes ignored.


To make mypy show error codes in error messages use :option:`--show-error-codes`.
See also the lists of :ref:`default error codes <error-code-list>` and
:ref:`optional error codes <error-codes-optional>`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should have a warning here, as ignoring some errors could be quite confusing and dangerous, especially misc.

@@ -439,6 +439,11 @@ These options may only be set in the global section (``[mypy]``).
``show_absolute_path`` (bool, default False)
Show absolute paths to files.

``ignore_error_codes`` (comma-separated list of strings)
Ignore errors with these error codes in given files or directories.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The part "in given files or directories" seems redundant. It's not included in other descriptions, and so it raises the question why this option applies to given files or directories, while other options don't apply to given files or directories?

``ignore_error_codes`` (comma-separated list of strings)
Ignore errors with these error codes in given files or directories.
See :option:`--ignore-error-codes <mypy --ignore-error-codes>` for
more details.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really tiny consistency nit: we use "for more information" about and "more details here". Maybe use "for more information" here as well.

error codes. Currently it's only possible to disable arbitrary error
codes on individual lines using this comment.
error codes. Also you can use :option:`--ignore-error-codes <mypy --ignore-error-codes>`
for more coarse-grained (per file or per directory) silencing of errors.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual command line flag only allows global silencing of errors, but this implies that it's per file or per directory. Either leave the parenthetical remark out, or somehow make it clear that per-file/directory options are only available through the config file or # mypy: ... comments. For consistency, I'd leave it out, since we probably don't want to discuss this separately for every reference to every option.

@@ -640,6 +641,9 @@ def add_invertible_flag(flag: str,
add_invertible_flag('--show-absolute-path', default=False,
help="Show absolute paths to files",
group=error_group)
error_group.add_argument(
'--ignore-error-codes', metavar='CODES', type=split_commas,
help="Ignore errors with these error codes (comma separated)")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling nit: use "comma-separated" (similar to above)

@@ -132,6 +133,9 @@ def __init__(self) -> None:
# Files in which to ignore all non-fatal errors
self.ignore_errors = False

# Ignore errors with these error codes in a given file.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: "in a given files." seems redunant. Also omit period at the end for consistency?

\[mypy]
ignore_error_codes = attr-defined, arg-type
\[mypy-b]
ignore_error_codes = name-defined, assignment
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to eventually move various strictness flags to be enabled/disabled through error codes. To do this, it seems that per-file options need to be a delta over global settings, instead of replacing them. Also, command-line options probably should be a delta over settings in the ini file. I.e., the ignored error codes would be the union of error codes ignored in the config file, on the command line, and in per-file options.

The logic gets a bit more involved once we allow enabling error codes as well, but at least we should be able to design the semantic for ignoring error codes in a way that we don't need to introduce a compatibility break.

not_defined # E: Name 'not_defined' is not defined
x: int = 'no' # E: Incompatible types in assignment (expression has type "str", variable has type "int")
'no'.no_way
def test(x: str) -> None: ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add daemon test case (to make sure the daemon applies these after updates).

@ilevkivskyi
Copy link
Member Author

Actually I decided not to do this. I will not have time, I will leave the branch if someone wants to work on this.

@ilevkivskyi ilevkivskyi closed this Dec 9, 2019
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