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.

Sign up for GitHub

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

Jump to bottom

New lint: Use Reverse with sort_by_key #5623

Merged
merged 11 commits into from
Jun 1, 2020
Merged

New lint: Use Reverse with sort_by_key #5623

merged 11 commits into from
Jun 1, 2020

Conversation

JarredAllen
Copy link
Contributor

@JarredAllen JarredAllen commented May 20, 2020 •
edited by phansch
Loading

Implements #5578

changelog: New lint: [unnecessary_sort_by]

camsteffen
camsteffen reviewed May 20, 2020
View reviewed changes
clippy_lints/src/sort_by_key_reverse.rs Outdated
Param { pat: Pat { kind: PatKind::Binding(_, _, b_ident, _), .. }, .. }
] = &closure_body.params;
if let ExprKind::MethodCall(method_path, _, [ref b_expr, ref a_expr]) = &closure_body.value.kind;
if method_path.ident.name.to_ident_string() == "cmp";
Copy link
Contributor

Choose a reason for hiding this comment

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

Or partial_cmp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm not mistaken, the sort_by_key method requires a function which returns something implementing Ord, whereas partial_cmp would only guarantee that the type implements PartialOrd. It would be possible to check if they're calling partial_cmp and the underlying type implements Ord, but I'm not sure if it would be worth it. How often do people use partial_cmp when they can use cmp?

Copy link
Contributor

@camsteffen camsteffen May 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed. Good point.

How often do people use partial_cmp when they can use cmp?

I dunno. It's nice to have but I can understand skipping it.

@camsteffen
Copy link
Contributor

camsteffen commented May 20, 2020 •
edited
Loading

It just occurred to me that we're skipping over a simpler lint. Could this be easily generalized to also cover cases of sort_by -> sort_by_key without Reverse? Then the lint could just be called sort_by_key.

@JarredAllen
Copy link
Contributor Author

That generalization would be pretty easy to do. I should be able to make that happen.

@phansch phansch added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label May 22, 2020
@JarredAllen
Copy link
Contributor Author

I've added that generalization, and I also made it detect that vec.sort_by(|a, b| a.cmp(b)) is better as vec.sort().

@flip1995 flip1995 requested a review from phansch May 25, 2020 16:52
@phansch phansch added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 26, 2020
@phansch
Copy link
Member

phansch commented May 27, 2020 •
edited
Loading

Re-opening to trigger a new build

@phansch phansch closed this May 27, 2020
@phansch phansch reopened this May 27, 2020
@camsteffen
Copy link
Contributor

Sorry to suggest another rename, but it may be good since the lint now covers sort_by -> sort in addition to sort_by -> sort_by_key. Ideas: simplify_sort or simplify_sort_by or unnecessary_sort_by.

phansch
phansch reviewed May 28, 2020
View reviewed changes
Copy link
Member

@phansch phansch left a comment

Choose a reason for hiding this comment

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

Overall this looks good to me, thanks!
Regarding the lint name I would probably opt for unnecessary_sort_by as @camsteffen suggests

clippy_lints/src/sort_by_key.rs Outdated Show resolved Hide resolved
clippy_lints/src/sort_by_key.rs Outdated Show resolved Hide resolved
clippy_lints/src/sort_by_key.rs Outdated Show resolved Hide resolved
clippy_lints/src/sort_by_key.rs Outdated Show resolved Hide resolved
clippy_lints/src/sort_by_key.rs Outdated Show resolved Hide resolved
@phansch phansch 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 May 28, 2020
@JarredAllen JarredAllen force-pushed the sort_by_key_reverse branch from 701a214 to 9a5baed Compare May 31, 2020 22:09
@JarredAllen
Copy link
Contributor Author

I think I just addressed those suggestions @phansch suggested

@JarredAllen
Copy link
Contributor Author

Ok, I missed re-running update_lints after renaming the lint, but now it should be good.

@phansch phansch added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jun 1, 2020
@phansch
Copy link
Member

phansch commented Jun 1, 2020

@bors r+ thanks!

@bors
Copy link
Contributor

bors commented Jun 1, 2020

📌 Commit b89880a has been approved by phansch

@bors
Copy link
Contributor

bors commented Jun 1, 2020

⌛ Testing commit b89880a with merge 1bfa0fa...

bors added a commit that referenced this pull request Jun 1, 2020
@bors
Auto merge of #5623 - JarredAllen:sort_by_key_reverse, r=phansch
1bfa0fa 
New lint: Use Reverse with sort_by_key

Implements  #5578
@bors
Copy link
Contributor

bors commented Jun 1, 2020

💔 Test failed - checks-action_test

@phansch
Copy link
Member

phansch commented Jun 1, 2020

@bors retry

@bors
Copy link
Contributor

bors commented Jun 1, 2020

⌛ Testing commit b89880a with merge bee0608...

@bors
Copy link
Contributor

bors commented Jun 1, 2020

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

@bors bors merged commit bee0608 into rust-lang:master Jun 1, 2020
@phansch phansch mentioned this pull request Jun 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camsteffen camsteffen camsteffen left review comments

@phansch phansch phansch left review comments

Assignees

@phansch phansch

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JarredAllen @camsteffen @phansch @bors