Set N818 to check error suffix on exception names. #157
+63
−3
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
|
I'm mildly -0 on this. I think there's a fair amount of variation in the community on this and if we add this, we'd want it to be ignored by default and only reported if someone |
I agree with you, there is a fair amount of variation, but also true that none current tool is being able to enforce this convention to normalize those exception names. If you think |
|
@sigmavirus24 Should I create a PR to add N818 into https://gitlab.com/pycqa/flake8/-/blob/master/src/flake8/defaults.py#L15? |
Nope. That's only for the 3 core tools Flake8 supports out of the box, not for plugins like this one |
|
@sigmavirus24 ok, let me know how to follow with this PR or if is something else I could help. |
jparise
reviewed
Nov 19, 2020
|
I ran this over a subset of Pinterest's (large) Python code base, and it flagged hundreds of cases. I see a lot of exception classes named with an "Exception" suffix. That's probably due to Java, etc. influences. I offer this as real-world anecdata rather than an endorsement or disapproval for inclusion in pep8-naming. If this was merged, we would probably not enable this rule by default because I think it's unlikely we'd go through a mass-renaming exercise for something like this; the perceived value of naming exception classes consistently isn't incredibly high. |
jparise
reviewed
Jan 5, 2021
There was a problem hiding this comment.
Thanks for your patience on this, @ecolell!
Here's one more round of code review feedback, and then we should make a final decision on whether this is something we want to include. I think if it was purely opt-in, we'd be good to go, but most folks will probably have it automatically enabled when they upgrade, so we should think through that.
Let's do it opt-in, and if at some point gets mainstream we update it 👍. |
|
@sigmavirus24, do you have a suggestion on the best way to achieve this?
I'm not yet familiar enough with that aspect of flake8's internal API to suggest an implementation. |
|
I'm pretty sure I documented this, but you can use these methods in the plugin when registering options for the plugin (do we call |
|
@ecolell could you take a crack at configuring this error code to be opt-in? |
|
pep8-naming/src/pep8ext_naming.py Lines 140 to 169 in 7cd85f8 extend_default_ignore
|
|
@jparise @sigmavirus24 Sorry the lag, I've updated the PR. I'm new on this, so please verify I did it right. |
sigmavirus24
approved these changes
Feb 16, 2021
|
Maybe I'm missing something, but when we use |
|
@sigmavirus24 do you have any insight into why |
|
I don't off the top of my head. I'd have to look into this |
|
Do you have logs with |
I don't know how to do it to make it work. Neither how to setup flake8 to use extension from the local folder (please point me the docs if are available). I would like to try to reproduce it and get more details as @sigmavirus24 required. |
You could create a virtualenv and then run |
|
@sigmavirus24 I've did |
|
Oh I think I know what's happening. We have our own |
sigmavirus24
added a commit
to PyCQA/flake8
that referenced
this pull request
Apr 15, 2021
Since Flake8 3.0 we've had the ability for plugins to use `extend_default_ignore` to register codes they want disabled by default. This, however, was a permanent disabling unfortunately. Our code didn't have a way of understanding that this new set of `ignore` codes was actually the 'default' set for that run. Much like the extended_select_list, we now attach extended_ignore_list to be able to confidently determine if the ignore we get in the DecisionEngine is actually the Default Ignore list and what plugins what us to ignore by default. Refs PyCQA/pep8-naming#157
|
PyCQA/flake8#1317 should fix this up for us. We might want to declare a minimum Flake8 version as a result. |
sigmavirus24
added a commit
to PyCQA/flake8
that referenced
this pull request
Apr 15, 2021
Since Flake8 3.0 we've had the ability for plugins to use `extend_default_ignore` to register codes they want disabled by default. This, however, was a permanent disabling unfortunately. Our code didn't have a way of understanding that this new set of `ignore` codes was actually the 'default' set for that run. Much like the extended_select_list, we now attach extended_ignore_list to be able to confidently determine if the ignore we get in the DecisionEngine is actually the Default Ignore list and what plugins what us to ignore by default. Refs PyCQA/pep8-naming#157
sigmavirus24
added a commit
to PyCQA/flake8
that referenced
this pull request
Apr 15, 2021
Since Flake8 3.0 we've had the ability for plugins to use `extend_default_ignore` to register codes they want disabled by default. This, however, was a permanent disabling unfortunately. Our code didn't have a way of understanding that this new set of `ignore` codes was actually the 'default' set for that run. Much like the extended_select_list, we now attach extended_ignore_list to be able to confidently determine if the ignore we get in the DecisionEngine is actually the Default Ignore list and what plugins what us to ignore by default. Refs PyCQA/pep8-naming#157
asottile
pushed a commit
to PyCQA/flake8
that referenced
this pull request
Apr 16, 2021
Since Flake8 3.0 we've had the ability for plugins to use `extend_default_ignore` to register codes they want disabled by default. This, however, was a permanent disabling unfortunately. Our code didn't have a way of understanding that this new set of `ignore` codes was actually the 'default' set for that run. Much like the extended_select_list, we now attach extended_ignore_list to be able to confidently determine if the ignore we get in the DecisionEngine is actually the Default Ignore list and what plugins what us to ignore by default. Refs PyCQA/pep8-naming#157
|
If 3.9.1 has this bug fix in it thanks to Anthony. If we put a |
|
@sigmavirus24 @jparise @asottile Thank you all for the feedback and support 👍. |
sampsyo
added a commit
to beetbox/beets
that referenced
this pull request
Jul 15, 2021
The new error <PyCQA/pep8-naming#157> strikes me as a little overzealous: while most exceptions are errors, there are some Exception subclasses that are *not* properly considered errors (e.g., when they're only a base class for other error classes).
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.
https://www.python.org/dev/peps/pep-0008/#exception-names