fix E721 types regex #1041
Merged
fix E721 types regex #1041
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Any `types.*Type*` matches incorrectly as a `type(...)` comparison; the regex `COMPARE_TYPE_REGEX` seems a bit too complicated for what should be a simple comparison case. Ref: https://github.com/PyCQA/pycodestyle/blob/main/pycodestyle.py#L147-L148 This reproduces the case in PyCQA#830
* `type(a) is type(b)` should still fail * same for `type(a) != type(b) or type(a) == type(ccc)` * We cannot assume `res == types.IntType` is wrong as the identity of the objects is not known at check time, either way it shouldn't be a E721 as it doesn't involve type(...) function as described in PEP8
| @@ -80,3 +78,8 @@ def func_histype(a, b, c): | |||
| pass | |||
| except Exception: | |||
| pass | |||
| #: Okay | |||
| from . import custom_types as types | |||
|
|
|||
There was a problem hiding this comment.
These test files aren't ever executed so we don't need a real module backing them here
There was a problem hiding this comment.
thanks, removed custom_types.py in 5df259b
sigmavirus24
approved these changes
Dec 18, 2021
|
reverting this for now so I can get a release out -- this caused significant churn -- I have a proposal in #1084 to make this more palatable when reintroducing it |
asottile
added a commit
that referenced
this pull request
Jul 30, 2022
Revert "Merge pull request #1041 from asfaltboy/issue-830-e721-types-regex-incorrect"
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
tl;dr The PEP8 example for the issue only includes the
type(a) == type(b)comparison, it does not include the== types.SomeTypeissue, the latter should not be handled under this error check.This check was introduced to trunk following issue #47, and I believe it incorrectly borrowed the example from the issue literally. The E721 check was since reported on a few false-positive issues and then expanded a number of times to handle those around types (see 034b9f4 and 0327e55).
I believe the behaviour should change because object comparison cannot be confirmed a "type comparison" in the same sense; not without having additional information, which is not possible with regex alone (maybe with AST analysis).
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).
If the community wants to keep it, I strongly advise it be made a separate warning level advisory (W###) rather than overloading E721, so we can handle any logic around it separately.
For reference, I learned more about the pattern, and tested for the cases using regexr:
fixes issue #830