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

allow is and is not for type comparisons (E721) #1086

Merged
merged 2 commits into from
Jul 15, 2023
Merged

Conversation

asottile
Copy link
Member

see #1084 and #1041 for more details

resolves #1084
resolves #830

@asottile asottile mentioned this pull request Jul 30, 2022
@@ -134,8 +134,10 @@
r'\s*(?(1)|(None|False|True))\b')
COMPARE_NEGATIVE_REGEX = re.compile(r'\b(?<!is\s)(not)\s+[^][)(}{ ]+\s+'
r'(in|is)\s')
COMPARE_TYPE_REGEX = re.compile(r'(?:[=!]=|is(?:\s+not)?)\s+type(?:s.\w+Type'
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find the rationale for removing types.*Type comparisons in the linked issues. As far as I can tell, those should still be included in the checks, shouldn't they?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think those got removed because the name types isn't necessarily the stdlib types module -- not 100% sure though

Copy link
Member Author

Choose a reason for hiding this comment

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

the original patch is here: #1041 -- this is just a revert-revert of that with an additional commit on top

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose the precise reasoning is the following, then:

This check also breaks many ligimitate cases (such as having your own types module with an Enum class for instance, and comparing a value to the enum member).

which I can relate to. You can, of course, also shadow the built-in type function, but that's just not something people do in general and having a custom types module is much more likely, although also ending your enum in Type is most likely less common.

@asottile asottile merged commit 2326e16 into main Jul 15, 2023
10 checks passed
@asottile asottile deleted the E721-allow-is-v2 branch July 15, 2023 16:11
@asfaltboy
Copy link
Contributor

@asottile my apologies for breaking the valid use cases in #1041, and thank you for following up with this fix and addressing the nuisance. 🙇

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.

E721 regression(s) E721: Naive for modules named types
3 participants