-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[refurb] Implement slice-to-remove-prefix-or-suffix
(FURB188
)
#13256
Conversation
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 |
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
FURB188 | 7 | 7 | 0 | 0 | 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Wow @AlexWaygood and @MichaReiser - what a fantastic and thorough code review! Thanks so much! Learning a lot from you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
Thanks again! I think I picked the remaining nits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This PR implements
FURB188
which suggests the use ofremoveprefix
/removesuffix
in order to remove prefixes and suffixes from strings, rather than slicing the string conditionally upon the output ofstartswith
/endswith
.Tested using same test suite as
refurb
.Moves the needle on #1348.