[refurb] Implement `slice-to-remove-prefix-or-suffix` (`FURB188`)#13256

Merged

AlexWaygood merged 26 commits intoastral-sh:mainfrom

dylwil3:remove-affix

Sep 9, 2024

Collaborator

dylwil3 commented Sep 5, 2024

This PR implements FURB188 which suggests the use of removeprefix/removesuffix in order to remove prefixes and suffixes from strings, rather than slicing the string conditionally upon the output of startswith/endswith.

Tested using same test suite as refurb.

Moves the needle on #1348.

dylwil3 added 6 commits

September 5, 2024 12:37


          add test fixture

8d429cc


          implement rule

b8c1831


          check rule at if stmts and if exprs

b4a7127


          snapshot

1308cb4


          update schema

28c3420


          adjust wording

ac5ea5a

Collaborator Author

dylwil3 commented Sep 5, 2024

Not super happy with how gnarly the implementation is - so open to any feedback/tips to clean it up. I've tried to add comments wherever possible so hopefully it is at least readable.

Definitely found myself wishing that Rust supported matching patterns involving structs with heavily nested Boxes... (rust-lang/rust#87121)

Contributor

github-actions bot commented Sep 5, 2024 •

edited

Loading

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+7 -0 violations, +0 -0 fixes in 3 projects; 51 projects unchanged)

bokeh/bokeh (+2 -0 violations, +0 -0 fixes)

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

+ src/bokeh/util/compiler.py:511:9: FURB188 [*] Prefer `removesuffix` over conditionally replacing with slice.
+ tests/support/plugins/file_server.py:63:9: FURB188 [*] Prefer `removeprefix` over conditionally replacing with slice.

rotki/rotki (+2 -0 violations, +0 -0 fixes)

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

+ rotkehlchen/assets/converters.py:57:5: FURB188 [*] Prefer `removesuffix` over conditionally replacing with slice.
+ rotkehlchen/exchanges/bitfinex.py:575:9: FURB188 [*] Prefer `removeprefix` over conditionally replacing with slice.

indico/indico (+3 -0 violations, +0 -0 fixes)

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

+ indico/cli/devserver.py:220:9: FURB188 [*] Prefer `removeprefix` over conditionally replacing with slice.
+ indico/core/db/sqlalchemy/colors.py:31:13: FURB188 [*] Prefer `removeprefix` over conditionally replacing with slice.
+ indico/modules/networks/fields.py:44:9: FURB188 [*] Prefer `removeprefix` over conditionally replacing with slice.

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
FURB188	7	7	0	0	0

AlexWaygood reviewed

View reviewed changes

Member

AlexWaygood left a comment

Nice! I can see that a lot of work went into this

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Show resolved Hide resolved

dylwil3 added 4 commits

September 7, 2024 13:43


          reduce nesting

6d65838


          import alias nit

c9c6092


          match builtin len more reliably

6db7f0a


          adjust u32::MAX skip logic

69bafad

MichaReiser reviewed

View reviewed changes

Member

MichaReiser left a comment

I only skimmed over the rust code and left a few NIT comments.

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

MichaReiser added rule preview labels

dylwil3 added 7 commits

September 8, 2024 10:58


          docstrings

e619ec7


          nits

984c479


          extend test fixture

7c58190


          typo in test fixture

9c2f530


          allow arbitrary expressions for text in affix removal

49c7668


          update snapshot

c7de848


          make fix via locator instead of generator

26661c3

Collaborator Author

dylwil3 commented Sep 9, 2024

Wow @AlexWaygood and @MichaReiser - what a fantastic and thorough code review! Thanks so much! Learning a lot from you

MichaReiser assigned AlexWaygood

AlexWaygood reviewed

View reviewed changes

Member

AlexWaygood left a comment

Very nice indeed! Basically just nits remaining, though there is one other location where I think you should use match_builtin_expr() for correctness

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Show resolved Hide resolved

dylwil3 added 4 commits

September 9, 2024 07:49


          Merge branch 'main' into remove-affix

4ed868a


          add shadow builtins test to fixture


          use semantic model over checker

3f30e64


          nits

6ce5fc2


          update snapshot

159f07c

Collaborator Author

dylwil3 commented Sep 9, 2024

Thanks again! I think I picked the remaining nits.


          clippy

e2a3ff4

AlexWaygood approved these changes

View reviewed changes

Member

AlexWaygood left a comment

Fab! Some tiny last remaining comments:

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

AlexWaygood reviewed

View reviewed changes

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Show resolved Hide resolved

AlexWaygood reviewed

View reviewed changes

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Outdated Show resolved Hide resolved

dylwil3 added 2 commits

September 9, 2024 09:53


          move builtin check to end of and statement and a few other small changes

29d234b


          copypasta error

1f60893

AlexWaygood approved these changes

View reviewed changes

Member

AlexWaygood left a comment

Thanks!

AlexWaygood reviewed

View reviewed changes

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Show resolved Hide resolved

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs Show resolved Hide resolved


          Apply suggestions from code review

add9317

AlexWaygood enabled auto-merge (squash)

September 9, 2024 15:03

AlexWaygood merged commit b04948f into astral-sh:main

dylwil3 deleted the remove-affix branch

September 9, 2024 15:42

BrewTestBot mentioned this pull request

ruff 0.6.5 Homebrew/homebrew-core#189287

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels