Skip to content

Deduplicate error codes for ignore-without-code#12194

Merged
hauntsaninja merged 4 commits into
python:masterfrom
cdce8p:improve-ignore-without-code
Feb 17, 2022
Merged

Deduplicate error codes for ignore-without-code#12194
hauntsaninja merged 4 commits into
python:masterfrom
cdce8p:improve-ignore-without-code

Conversation

@cdce8p
Copy link
Copy Markdown
Collaborator

@cdce8p cdce8p commented Feb 17, 2022

Description

#11633 added the awesome ignore-without-code option. During testing I noticed that in some cases an error code would be displayed multiple times. This can be rather annoying, since it's only necessary to ignore an error code once.

# current
error: "type: ignore" comment without error code (currently ignored: [union-attr, arg-type, union-attr])
# new
error: "type: ignore" comment without error code (currently ignored: [arg-type, union-attr])

/CC: @PeterJCLaw

Test Plan

Added a new test case.

@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks!

Separately, I found
"type: ignore" comment without error code (currently ignored: [arg-type, union-attr])
just a little confusing. Maybe something like:
"type: ignore" comment without error code (use "type: ignore[arg-type, union-attr]" instead)
is clearer?

Comment thread mypy/errors.py Outdated
@cdce8p
Copy link
Copy Markdown
Collaborator Author

cdce8p commented Feb 17, 2022

Thanks!

Separately, I found "type: ignore" comment without error code (currently ignored: [arg-type, union-attr]) just a little confusing. Maybe something like: "type: ignore" comment without error code (use "type: ignore[arg-type, union-attr]" instead) is clearer?

Think that would make sense. I could update this PR tomorrow or open a new one.

@github-actions

This comment has been minimized.

@hauntsaninja
Copy link
Copy Markdown
Collaborator

Let's just update this one :-)

@cdce8p cdce8p force-pushed the improve-ignore-without-code branch from 7319126 to 02aa0c3 Compare February 17, 2022 07:58
@github-actions

This comment has been minimized.

1 similar comment
@github-actions
Copy link
Copy Markdown
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@hauntsaninja hauntsaninja merged commit 5b4ffa9 into python:master Feb 17, 2022
@hauntsaninja
Copy link
Copy Markdown
Collaborator

Thank you!

@cdce8p cdce8p deleted the improve-ignore-without-code branch February 17, 2022 08:59
@PeterJCLaw
Copy link
Copy Markdown
Contributor

PeterJCLaw commented Feb 17, 2022

Separately, I found "type: ignore" comment without error code (currently ignored: [arg-type, union-attr]) just a little confusing. Maybe something like: "type: ignore" comment without error code (use "type: ignore[arg-type, union-attr]" instead) is clearer?

For context: it was intentional not to have the error message just tell you to update the ignore, on the grounds that it's likely users would have existing ignores that may not be quite right and just updating the ignore comment blindly may not be a great idea (implemented in 6f39906). The message was thus aimed at making the process of converting the ignores into more specific ones needing just a bit more human input, to encourage review at the same time.
I still think that's useful, though very open to other spellings of the message :)

@cdce8p
Copy link
Copy Markdown
Collaborator Author

cdce8p commented Feb 17, 2022

Separately, I found "type: ignore" comment without error code (currently ignored: [arg-type, union-attr]) just a little confusing. Maybe something like: "type: ignore" comment without error code (use "type: ignore[arg-type, union-attr]" instead) is clearer?

For context: it was intentional not to have the error message just tell you to update the ignore, on the grounds that it's likely users would have existing ignores that may not be quite right and just updating the ignore comment blindly may not be a great idea (implemented in 6f39906). The message was thus aimed at making the process of converting the ignores into more specific ones needing just a bit more human input, to encourage review at the same time. I still think that's useful, though very open to other spellings of the message :)

It's a good point. Although from my test updating 100+ ignores, it wasn't all that practical tbh. It's a rather large codebase with only partial annotated code. The main benefit I see with it is not ignoring any new type errors that might come up once you start adding more annotations. That's just my opinion though. If there is a desire to revert it, I wouldn't oppose it.

@hauntsaninja
Copy link
Copy Markdown
Collaborator

Do you feel the following captures the spirit of the original better?
"type: ignore" comment without error code (consider "type: ignore[arg-type, union-attr]" instead)

@cdce8p
Copy link
Copy Markdown
Collaborator Author

cdce8p commented Feb 19, 2022

Opened a followup PR: #12216

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.

3 participants