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

remove Y092 #88

Merged
merged 2 commits into from
Jan 17, 2022
Merged

remove Y092 #88

merged 2 commits into from
Jan 17, 2022

Conversation

JelleZijlstra
Copy link
Collaborator

Disabled-by-default error codes are problematic; see #75.

Y092 finds a fairly harmless error and has significant false positives (python/typeshed#6930).
I'm removing it so we can get rid of disabled-by-default errors and simplify our
setup.

Disabled-by-default error codes are problematic; see #75.

Y092 finds a fairly harmless error and has significant false positives (python/typeshed#6930).
I'm removing it so we can get rid of disabled-by-default errors and simplify our
setup.
@Akuli
Copy link
Collaborator

Akuli commented Jan 16, 2022

Why not just make it turned on by default, and use # noqa in the 48 lines of typeshed where = ... is needed?

@JelleZijlstra
Copy link
Collaborator Author

I feel like the signal-to-noise ratio is too low. Checks that need to be disabled half the time they trigger aren't useful. And that argument becomes stronger when the benefit of enabling the check is lower, and there is very little benefit to removing the = ....

@AlexWaygood
Copy link
Collaborator

Why not just make it turned on by default, and use # noqa in the 48 lines of typeshed where = ... is needed?

That's an awful lot of typing and typing_extensions. I think it would make the code awfully ugly.

@Akuli
Copy link
Collaborator

Akuli commented Jan 16, 2022

Apart from typing and typing_extensions, this seems to be the only use case where = ... is really needed:

working_set: WorkingSet = ...
require = working_set.require

I have seen lots of typeshed reviews where the reviewer (usually srittau, sometimes someone else I think) mentions about = ... being redundant. I think it would be nice to automate this, even if one relatively rare corner case remains. (outside typing and typing_extensions which are special-cased anyway).

@JelleZijlstra
Copy link
Collaborator Author

python/typeshed#6930 has two possible approaches for getting rid of the false positives.

@Akuli
Copy link
Collaborator

Akuli commented Jan 16, 2022

Checks that need to be disabled half the time they trigger aren't useful.

Half of what exactly? Typeshed contains relatively few occurrences of = ..., because we usually mention them in reviews. If we didn't, there would have been more unnecessary = ... removed in python/typeshed#6930.

Copy link
Collaborator

@Akuli Akuli left a comment

Choose a reason for hiding this comment

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

I think it's fine to remove this for now, mostly because disabled-by-default checks are problematic, but I would like to know what @srittau thinks before we merge.

@srittau
Copy link
Collaborator

srittau commented Jan 17, 2022

I'm not too happy about this, since this is a poster-child for a linter check. This is something I need to manually check and complain about, which should be handled by a linter. That said, since we're not using the check in typeshed at the moment, and due to the practical problems with it, I'm okay with removing it for now, although we should leave an issue for eventual re-adding it.

@Akuli
Copy link
Collaborator

Akuli commented Jan 17, 2022

Feel free to remove, once merge conflict is fixed :)

@srittau srittau merged commit 4e40763 into master Jan 17, 2022
@srittau srittau deleted the y092 branch January 17, 2022 17:55
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.

4 participants