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

add trim_split_whitespace #8575

Merged
merged 1 commit into from
May 4, 2022
Merged

Conversation

FoseFx
Copy link
Contributor

@FoseFx FoseFx commented Mar 22, 2022

Closes #8521

changelog: [trim_split_whitespace]

@rust-highfive
Copy link

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 22, 2022
clippy_lints/src/strings.rs Outdated Show resolved Hide resolved
clippy_lints/src/strings.rs Outdated Show resolved Hide resolved
clippy_lints/src/strings.rs Outdated Show resolved Hide resolved
clippy_lints/src/strings.rs Outdated Show resolved Hide resolved
clippy_lints/src/strings.rs Show resolved Hide resolved
tests/ui/trim_split_whitespace.rs Show resolved Hide resolved
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 24, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 25, 2022
…, r=flip1995

add diagnostic items for clippy's `trim_split_whitespace`

Adding the following diagnostic items:
 * str_split_whitespace,
 * str_trim,
 * str_trim_start,
 * str_trim_end

They are needed for rust-lang/rust-clippy#8575

r? `@flip1995`
@flip1995
Copy link
Member

flip1995 commented Mar 25, 2022

To use the diagnostic items in this PR, you can try to bump the nightly version in the rust-toolchain file to tomorrow 2022-03-26. I think that should work without breaking anything else in Clippy.

(You can only do this once this nightly is released in about 4-7 hours, obviously)

@FoseFx
Copy link
Contributor Author

FoseFx commented Mar 26, 2022

There is a different test failing on nightly-2022-03-26. I guess I wait until that is sorted out on master?

@bors
Copy link
Contributor

bors commented Mar 30, 2022

☔ The latest upstream changes (presumably #8576) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Apr 4, 2022

☔ The latest upstream changes (presumably #8594) made this pull request unmergeable. Please resolve the merge conflicts.

@flip1995 flip1995 added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 5, 2022
@flip1995
Copy link
Member

flip1995 commented Apr 5, 2022

Blocked on the sync on Thursday

@FoseFx FoseFx requested a review from flip1995 April 7, 2022 17:14
@flip1995 flip1995 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work labels Apr 7, 2022
@ghost
Copy link

ghost commented Apr 8, 2022

Looks like the tests are missing the edge case where the type implements Deref<Target=str> and has it's own split_whitespace but does not have it's own trim.

Otherwise it's looking good. I tested on a bunch of top crates. I found 3 warnings in procfs and fix worked.

---> procfs-0.12.0/src/net.rs:570:27                                  
warning: found call to `str::trim` before `str::split_whitespace`     
   --> src/net.rs:570:27
    |
570 |         let mut split = s.trim().split_whitespace();
    |                           ^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove `trim()`: `split_whitespace()`
    |
    = note: requested on the command line with `-W clippy::trim-split-whitespace`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#trim_split_whitespace


---> procfs-0.12.0/src/diskstats.rs:103:26                            
warning: found call to `str::trim` before `str::split_whitespace`     
   --> src/diskstats.rs:103:26
    |
103 |         let mut s = line.trim().split_whitespace();
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove `trim()`: `split_whitespace()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#trim_split_whitespace


---> procfs-0.12.0/src/locks.rs:146:26                                
warning: found call to `str::trim` before `str::split_whitespace`     
   --> src/locks.rs:146:26
    |
146 |         let mut s = line.trim().split_whitespace();
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove `trim()`: `split_whitespace()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#trim_split_whitespace

@FoseFx FoseFx force-pushed the trim_split_whitespace2 branch 3 times, most recently from 52c6157 to 1216903 Compare April 13, 2022 16:04
@bors
Copy link
Contributor

bors commented Apr 14, 2022

☔ The latest upstream changes (presumably #8626) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Apr 19, 2022

☔ The latest upstream changes (presumably #8711) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Apr 25, 2022

☔ The latest upstream changes (presumably #8727) made this pull request unmergeable. Please resolve the merge conflicts.

@FoseFx
Copy link
Contributor Author

FoseFx commented May 4, 2022

poke

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

LGTM me overall. Small suggestion on how to improve the suggestion.

clippy_lints/src/strings.rs Outdated Show resolved Hide resolved
clippy_lints/src/strings.rs Outdated Show resolved Hide resolved
tests/ui/trim_split_whitespace.fixed Outdated Show resolved Hide resolved
tests/ui/trim_split_whitespace.fixed Outdated Show resolved Hide resolved
@FoseFx
Copy link
Contributor Author

FoseFx commented May 4, 2022

👍

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

Can you squash the commits? After that, this is ready to get merged.

@FoseFx
Copy link
Contributor Author

FoseFx commented May 4, 2022

Done 👍

@flip1995
Copy link
Member

flip1995 commented May 4, 2022

Thanks! Sorry reviews take so long currently...

@bors r+

@bors
Copy link
Contributor

bors commented May 4, 2022

📌 Commit fea177f has been approved by flip1995

@bors
Copy link
Contributor

bors commented May 4, 2022

⌛ Testing commit fea177f with merge c7a705a...

@bors
Copy link
Contributor

bors commented May 4, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing c7a705a to master...

@bors bors merged commit c7a705a into rust-lang:master May 4, 2022
@FoseFx FoseFx deleted the trim_split_whitespace2 branch July 20, 2022 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New lint: calling trim() then split_whitespace()
4 participants