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

[refurb] Mark fix as unsafe when the right-hand side is a string (FURB171) #15273

Merged
merged 2 commits into from
Jan 5, 2025

Conversation

InSyncWithFoo
Copy link
Contributor

Summary

Resolves #15268.

Test Plan

cargo nextest run and cargo insta test.

Copy link
Contributor

github-actions bot commented Jan 5, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -0 violations, +0 -4 fixes in 2 projects; 53 projects unchanged)

zulip/zulip (+0 -0 violations, +0 -4 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ tools/lib/template_parser.py:637:29: FURB171 Membership test against single-item container
- tools/lib/template_parser.py:637:29: FURB171 [*] Membership test against single-item container
+ tools/lib/template_parser.py:645:29: FURB171 Membership test against single-item container
- tools/lib/template_parser.py:645:29: FURB171 [*] Membership test against single-item container

python-trio/trio (+0 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview


Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
FURB171 4 0 0 0 4

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Nice, thanks! My only comment is that I wonder if we should even be emitting this lint if the r.h.s. is a string? It's quite possible that the user is deliberately exploiting the way in works with regard to string here, in which case it would be annoying to have to noqa the lint away.

That does seem unlikely, though, so maybe it's better to continue emitting the lint and just mark the fix as unsafe. Not sure -- what do you think?

@InSyncWithFoo
Copy link
Contributor Author

InSyncWithFoo commented Jan 5, 2025

I'm with the latter. In such cases, c == "" or c == "a" (or c in ("", "a") to avoid double evaluation) would be clearer and I would have suggested that as a secondary fix had it not been for the one-fix-per-diagnostic limit.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks!

@AlexWaygood AlexWaygood added bug Something isn't working fixes Related to suggested fixes for violations preview Related to preview mode features labels Jan 5, 2025
@AlexWaygood AlexWaygood merged commit 00aa387 into astral-sh:main Jan 5, 2025
21 checks passed
@InSyncWithFoo InSyncWithFoo deleted the FURB171 branch January 5, 2025 17:54
dcreager added a commit that referenced this pull request Jan 6, 2025
* main: (60 commits)
  [`ruff`] Dataclass enums (`RUF049`) (#15299)
  Better error message when `--config` is given a table key and a non-inline-table value (#15266)
  Update pre-commit dependencies (#15289)
  Don't fix in ecosystem check (#15267)
  Update Rust crate itertools to 0.14.0 (#15287)
  Remove accidental empty block at the bottom of `split-static-string (SIM905)` doc (#15290)
  Update Rust crate clearscreen to v4 (#15288)
  Update Rust crate insta to v1.42.0 (#15286)
  Update NPM Development dependencies (#15285)
  Update dependency uuid to v11.0.4 (#15284)
  Update dependency ruff to v0.8.6 (#15283)
  Update Rust crate syn to v2.0.95 (#15282)
  Update Rust crate matchit to v0.8.6 (#15281)
  Update Rust crate bstr to v1.11.3 (#15280)
  [red-knot] Future-proof `Type::is_disjoint_from()` (#15262)
  [red-knot] Improve `Type::is_disjoint_from()` for `KnownInstanceType`s (#15261)
  [red-knot] Minor simplifications and improvements to constraint narrowing logic (#15270)
  Allow assigning ellipsis literal as parameter default value (#14982)
  [red-knot] fix control flow for assignment expressions in elif tests (#15274)
  [`refurb`] Mark fix as unsafe when the right-hand side is a string (`FURB171`) (#15273)
  ...
dcreager added a commit that referenced this pull request Jan 6, 2025
* main: (29 commits)
  [`ruff`] Dataclass enums (`RUF049`) (#15299)
  Better error message when `--config` is given a table key and a non-inline-table value (#15266)
  Update pre-commit dependencies (#15289)
  Don't fix in ecosystem check (#15267)
  Update Rust crate itertools to 0.14.0 (#15287)
  Remove accidental empty block at the bottom of `split-static-string (SIM905)` doc (#15290)
  Update Rust crate clearscreen to v4 (#15288)
  Update Rust crate insta to v1.42.0 (#15286)
  Update NPM Development dependencies (#15285)
  Update dependency uuid to v11.0.4 (#15284)
  Update dependency ruff to v0.8.6 (#15283)
  Update Rust crate syn to v2.0.95 (#15282)
  Update Rust crate matchit to v0.8.6 (#15281)
  Update Rust crate bstr to v1.11.3 (#15280)
  [red-knot] Future-proof `Type::is_disjoint_from()` (#15262)
  [red-knot] Improve `Type::is_disjoint_from()` for `KnownInstanceType`s (#15261)
  [red-knot] Minor simplifications and improvements to constraint narrowing logic (#15270)
  Allow assigning ellipsis literal as parameter default value (#14982)
  [red-knot] fix control flow for assignment expressions in elif tests (#15274)
  [`refurb`] Mark fix as unsafe when the right-hand side is a string (`FURB171`) (#15273)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixes Related to suggested fixes for violations preview Related to preview mode features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FURB171: Potentially unsafe fix
2 participants