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

B905's autofix sets strict=False, but the suggested fix is strict=True #13581

Closed
mdbernard opened this issue Oct 1, 2024 · 3 comments · Fixed by #13656
Closed

B905's autofix sets strict=False, but the suggested fix is strict=True #13581

mdbernard opened this issue Oct 1, 2024 · 3 comments · Fixed by #13656
Labels
rule Implementing or modifying a lint rule

Comments

@mdbernard
Copy link
Contributor

mdbernard commented Oct 1, 2024

The docs for B905 state the suggested fix is to use strict=True, but the autofix introduced here autofixes with strict=False.

Either the docs or the autofix should be updated to match the other.

I would argue that the autofix should be changed to strict=True, since that is the option less likely to cause unexpected runtime behavior for users. It might cause runtime exceptions to be raised, but I personally would prefer those to silently continuing with potentially unintended behavior.


Keywords: B905, zip-without-explicit-strict

Current ruff version: 0.6.6

Minimal snippet:

zip(a, b)

Command invoked:
ruff check example.py --target-version=py311 --select=B905

Output:

example.py:1:1: B905 [*] `zip()` without an explicit `strict=` parameter
  |
1 | zip(a, b)
  | ^^^^^^^^^ B905
  |
  = help: Add explicit `strict=False`

Found 1 error.
[*] 1 fixable with the `--fix` option.

Expected output:

...
  = help: Add explicit `strict=True`
...
@zanieb
Copy link
Member

zanieb commented Oct 1, 2024

I wonder if we use strict=False to avoid changing the semantics / behavior of the code? I think this is probably correct, but we should recommend adding strict not necessarily strict=False in the message?

@zanieb zanieb added the rule Implementing or modifying a lint rule label Oct 1, 2024
@mdbernard
Copy link
Contributor Author

That also works! Just removing the autofix altogether and letting the user decide what they want. Based on personal experience I'd almost always want strict=True, but I see the counter-argument that it changes behavior, so it might not be a desirable autofix.

@zanieb
Copy link
Member

zanieb commented Oct 1, 2024

I don't think we should remove the autofix entirely though, just change the message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants