Skip to content

Stuttter lint #994

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

Merged
merged 8 commits into from
Jun 16, 2016
Merged

Stuttter lint #994

merged 8 commits into from
Jun 16, 2016

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 8, 2016

fixes #904

@mcarton
Copy link
Member

mcarton commented Jun 8, 2016

I'm not sure I like most of the fallout for this one 😕.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 8, 2016

jup :) the function ones are pretty bad

but some function things like utils::conf::conf_file i found to be a good change... so maybe we limit it to public types/functions?

} else {
HirVec::new()
}
}));
Copy link
Member

Choose a reason for hiding this comment

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

Why removing the function here? I like closures for small stuffs but not so much for longer functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well I can change it back to a function, but we should maybe consider a lint that lints .filter_map(x).flat_map(y) and .filter(z).flat_map(y)

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 10, 2016

updated to not lint on private items.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 10, 2016

apparently we need another rustup

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 13, 2016

travis hickuped, but cargo test runs through locally

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 16, 2016

rebased, but sth is wrong with regex-macros

@mcarton
Copy link
Member

mcarton commented Jun 16, 2016

It's been a few days, I've made a PR but they haven't published a new version.

@llogiq
Copy link
Contributor

llogiq commented Jun 16, 2016

They just did. @mcarton do you want to update our dependencies?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 16, 2016

travis likes this now

@mcarton
Copy link
Member

mcarton commented Jun 16, 2016

But I don't 😧. IMO the potential for false positive is still huge.
@llogiq, @Manishearth ?

@Manishearth
Copy link
Member

I'm ... not sure. I don't like the fallout. I'm okay with Allow.

@llogiq
Copy link
Contributor

llogiq commented Jun 16, 2016

I'm also a bit worried about swamping the output. The severity of the reported problem is low, and the chance for this lint to trigger is high. So even with a fairly low rate of false positive, we will still swamp out reports of higher-severity problems.

As an #[allow]ed lint, it'd be a no-brainer to merge.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 16, 2016

done

@llogiq llogiq merged commit f3397af into rust-lang:master Jun 16, 2016
@llogiq
Copy link
Contributor

llogiq commented Jun 16, 2016

Thanks.

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.

Stuttter lint
4 participants