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

Change B038 to B909 and make it optional #456

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

mimre25
Copy link
Contributor

@mimre25 mimre25 commented Jan 22, 2024

While writing this up, I was thinking about making this optional and have two questions:

  1. Should we change the B code (as I did)?
  2. What would you say are the guidelines for making a rule optional?

B038 lead to some false positives that stem from methods defined in the standard library that have the same name as mutating functions for container types like lists and dicts.
Thus we decided to make this rule optional.
See #455 for the related discussion.

Closes #455

B038 lead to some false positives that stem from methods defined in the
standard library that have the same name as mutating functions for
container types like lists and dicts.
Thus we decided to make this rule optional.
See PyCQA#455 for the related
discussion.
@cooperlees
Copy link
Collaborator

cooperlees commented Jan 23, 2024

Should we change the B code (as I did)?

Yes, but lets mark 38 as to skip / not use as people might have it already blocked in their configs now ...

What would you say are the guidelines for making a rule optional?

Good question, we've always just been adhoc. Generally if there's a good chance majority of people don't need / want / the lint will be too noisy unless helped / tweaked it should be optional.

Happy for others to chime in with a more concrete definition. We can add it into the README etc. if we want too.

I'll try review this today..

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

LGTM - I just wonder if we should care about 'denylisting' b038 or just try to remember to not use it for next new check?

@FozzieHi
Copy link
Contributor

LGTM - I just wonder if we should care about 'denylisting' b038 or just try to remember to not use it for next new check?

When B028 was renamed to B907, we reused B028. Not sure if we can get any analytics on whether that was the correct decision or not (i.e. how many people still have B028 in their ignore list), but I definitely think that if we decide to skip it, it should be included in the code base without any implementation so we don't forget.

@FozzieHi
Copy link
Contributor

Just noticed this when looking at this PR, but B907 isn't included in the disabled_by_default list for some reason. Maybe it was forgotten during the rename, should we just throw that in for this PR?

I'm curious at how the noise was reduced when it isn't even in the list, is it somehow still disabled anyway?

@JelleZijlstra
Copy link
Collaborator

I'd prefer to not reuse the code, we're in no danger of running out of error codes. We can introduce some mechanism in the code to make sure the code stays reserved.

@FozzieHi
Copy link
Contributor

Just noticed this when looking at this PR, but B907 isn't included in the disabled_by_default list for some reason. Maybe it was forgotten during the rename, should we just throw that in for this PR?

I'm curious at how the noise was reduced when it isn't even in the list, is it somehow still disabled anyway?

I've looked into this some more, and it seems like the should_warn function is preventing all B9 rules from warning by default. The function itself is a workaround (see the function comments), so I still think it's a good idea to add all of the B9 rules manually to the disabled_by_default list.

@cooperlees cooperlees merged commit 82357df into PyCQA:main Feb 7, 2024
7 checks passed
@mimre25 mimre25 mentioned this pull request Feb 8, 2024
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.

B038 false positives
4 participants