Skip to content

[flake8-bugbear] Ignore B028 if skip_file_prefixes is present #18047

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.

Sign up for GitHub

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

Merged
dylwil3 merged 4 commits into from
May 12, 2025
Merged

[flake8-bugbear] Ignore B028 if skip_file_prefixes is present #18047

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1268c77
[`flake8-bugbear`] Ignore `B028` if `skip_file_prefixes` is present
LaBatata101 May 12, 2025
acdfd83
update
LaBatata101 May 12, 2025
a593744
check for nonempty tuple
LaBatata101 May 12, 2025
c278592
Update logic
LaBatata101 May 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B028.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,6 @@
# some comments here
source = None # no trailing comma
)

# https://github.com/astral-sh/ruff/issues/18011
warnings.warn("test", skip_file_prefixes=(os.path.dirname(__file__),))
6 changes: 6 additions & 0 deletions crates/ruff_linter/src/rules/flake8_bugbear/rules/no_explicit_stacklevel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,16 @@ pub(crate) fn no_explicit_stacklevel(checker: &Checker, call: &ast::ExprCall) {
return;
}

// Signature as of Python 3.13 (https://docs.python.org/3/library/warnings.html#warnings.warn)
// ```text
// 0 1 2 3 4
// warnings.warn(message, category=None, stacklevel=1, source=None, *, skip_file_prefixes=())
// ```
Comment thread
LaBatata101 marked this conversation as resolved.
if call
.arguments
.find_argument_value("stacklevel", 2)
.is_some()
|| call.arguments.find_keyword("skip_file_prefixes").is_some()

@dylwil3 dylwil3 May 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think you have to check that the value is a nonempty tuple of strings. Could you also add a test for if a user directly specifies skip_file_prefixes = () - we should emit the lint in that case, when the stacklevel is not set.

@dylwil3 dylwil3 May 12, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(If the expression is not a literal tuple of strings, then I think it's okay to skip the lint, even though that might be a false negative).

@LaBatata101 LaBatata101 May 12, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we need to check if the tuple is nonempty, because if we check for a tuple of strings, this case warnings.warn("test", skip_file_prefixes=(os.path.dirname(__file__),)) will be flagged.

@dylwil3 dylwil3 May 12, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sorry maybe I worded that in a confusing way:

  1. If the keyword argument is used, and the value is not a literal tuple of strings, then skip the lint.
  2. If the keyword argument is used, and the value is a literal tuple of strings, skip if and only if the tuple is nonempty.

The logic you have in the most recent commit will give a false positive in the case where the user does something like this:

_my_prefixes = ("this","that")
warnings.warn("test", skip_file_prefixes = _my_prefixes)

Could you add this as another test case and take another stab at the implementation logic?

@LaBatata101 LaBatata101 May 12, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've updated the implementation logic

|| call
.arguments
.args
Expand Down
2 changes: 2 additions & 0 deletions ...les/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B028_B028.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,5 @@ B028.py:22:1: B028 [*] No explicit `stacklevel` keyword argument found
26 |- source = None # no trailing comma
26 |+ source = None, stacklevel=2 # no trailing comma
27 27 | )
28 28 |
29 29 | # https://github.com/astral-sh/ruff/issues/18011