Skip to content

B906: Add visit_Bytes, visit_Num and visit_Str to the list of ignored visit_* functions#338

Merged
cooperlees merged 1 commit intoPyCQA:mainfrom
AlexWaygood:b096-false-positives
Jan 28, 2023
Merged

B906: Add visit_Bytes, visit_Num and visit_Str to the list of ignored visit_* functions#338
cooperlees merged 1 commit intoPyCQA:mainfrom
AlexWaygood:b096-false-positives

Conversation

@AlexWaygood
Copy link
Copy Markdown
Contributor

@AlexWaygood AlexWaygood commented Jan 21, 2023

Hi, thanks for flake8-bugbear! I'm a maintainer of the flake8-pyi plugin for flake8, and I'd love to switch on B906 for our codebase, as I think it's a great idea for a flake8-bugbear check. Unfortunately, however, there's still a false-positive if I switch B906 on over at flake8-pyi, even with the latest release (which includes #335).

The false positive is emitted due to the fact that we have a visit_Str method in our ast.NodeVisitor subclass. ast.Str has been deprecated since 3.8, but it's necessary for us to define a visit_Str method nonetheless in order to maintain backwards compatibility with Python 3.7.

ast.Str nodes have a non-empty _fields attribute, but they can't contain any ast subnodes that could be visited, much the same as ast.alias, ast.Constant, or any of the other nodes special-cased. The same applies for two other deprecated nodes: ast.Num and ast.Bytes. This PR fixes the false positive on flake8-pyi's codebase by adding ast.Str, ast.Num and ast.Bytes to the list of AST nodes special-cased by B906.

Copy link
Copy Markdown
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.

Thanks! Can't hurt to exempt these for triggering since we still support 3.7. Just so I understand correctly we can deprecate once we're >= 3.8 right?

@AlexWaygood
Copy link
Copy Markdown
Contributor Author

AlexWaygood commented Jan 22, 2023

Just so I understand correctly we can deprecate once we're >= 3.8 right?

Strictly speaking, "we promise our plugin will work when run on Python 3.7" and "we promise that our plugin will not emit false positives when checking other code that supports being run on Python 3.7" are two different things. However, I think it's fine to drop support for other codebases supporting 3.7 at the same time as you yourself drop support for 3.7 :)

That's obviously your decision to make, though!

@cooperlees
Copy link
Copy Markdown
Collaborator

That's obviously your decision to make, though!

Cool. I feel if people choose to stay on old versions of python, they can stay on old versions of libraries + tools too ...

Copy link
Copy Markdown
Contributor

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@cooperlees cooperlees merged commit a70f0dd into PyCQA:main Jan 28, 2023
@AlexWaygood AlexWaygood deleted the b096-false-positives branch January 28, 2023 20:16
@AlexWaygood
Copy link
Copy Markdown
Contributor Author

Thanks folks!

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.

3 participants