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

Implement cargo lint and fix some clippy errors #1374

Merged
merged 25 commits into from
Jun 4, 2019
Merged

Conversation

alanhdu
Copy link
Contributor

@alanhdu alanhdu commented Jun 4, 2019

This creates a cargo lint command that runs clippy with certain lints disabled. I've also gone ahead and fixed some of the lint errors, although there are many more still to go.

cc #848

Copy link
Member

@matklad matklad left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

It all looks good to me except the one lint that suggest flipping if let Err(_) = foo to if foo.is_err(), which I don't really agree with :-)

r=me with that lint silenced!

bors delegate+

@@ -334,7 +334,7 @@ fn best_action_for_target<'b, 'a: 'b>(
.filter_map(ast::UseItem::use_tree)
.map(|u| walk_use_tree_for_best_action(&mut storage, None, u, target))
.fold(None, |best, a| {
best.and_then(|best| Some(*ImportAction::better(&best, &a))).or(Some(a))
best.and_then(|best| Some(*ImportAction::better(&best, &a))).or_else(|| Some(a))
Copy link
Member

Choose a reason for hiding this comment

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

I feel conflicted about this, it seems like closure creates noise for little benefit here. But I guess it's ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think the fact that clippy interprets these constructors as "function calls" is pretty unfortunate, It looks like a bug (rust-lang/rust-clippy#1609), but for now I figured that it'd be better to make the changes just so we can get all the lint errors out of the way.

@@ -183,7 +183,7 @@ impl ProjectWorkspace {
if let (Some(&from), Some(&to)) =
(sysroot_crates.get(&from), sysroot_crates.get(&to))
{
if let Err(_) = crate_graph.add_dep(from, name.into(), to) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's disable this lint? I find if let somewhat more readable, and, with result, Err(_) also sends a strong signal that an error is ignored.

@bors
Copy link
Contributor

bors bot commented Jun 4, 2019

✌️ alanhdu can now approve this pull request

@alanhdu
Copy link
Contributor Author

alanhdu commented Jun 4, 2019

bors r+

bors bot added a commit that referenced this pull request Jun 4, 2019
1374: Implement `cargo lint` and fix some clippy errors r=alanhdu a=alanhdu

This creates a `cargo lint` command that runs clippy with certain lints disabled. I've also gone ahead and fixed some of the lint errors, although there are many more still to go.

cc #848 

Co-authored-by: Alan Du <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jun 4, 2019

Build succeeded

@bors bors bot merged commit aa30c49 into rust-lang:master Jun 4, 2019
@alanhdu alanhdu deleted the clippy branch June 4, 2019 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants