Skip to content

Add a configuration file and a POC BLACKLISTED_NAME lint #698

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
Merged
merged 11 commits into from
Mar 13, 2016
Merged

Add a configuration file and a POC BLACKLISTED_NAME lint #698

merged 11 commits into from
Mar 13, 2016

Conversation

mcarton
Copy link
Member

@mcarton mcarton commented Feb 22, 2016

As suggested in #694. Also add a BLACKLISTED_NAME lint and adapt the TYPE_COMPLEXITY and CYCLOMATIC_COMPLEXITY lints.

The wiki entry looks like this:

blacklisted_name

Default level: Warn

What it does: This lints about usage of blacklisted names.

Why is this bad? These names are usually placeholder names and should be avoided.

Known problems: None.

Example: let foo = 3.14;

Configuration: This lint has the following configuration variables:

  • blacklisted-names: Vec<String>: The list of blacklisted names to lint about (defaults to ["foo", "bar", "baz"]).

@mcarton mcarton force-pushed the conf branch 3 times, most recently from 22c3f4b to 244a0da Compare February 29, 2016 16:23
@mcarton
Copy link
Member Author

mcarton commented Feb 29, 2016

Any comment?

@mcarton mcarton mentioned this pull request Mar 5, 2016
Open
@mcarton
Copy link
Member Author

mcarton commented Mar 6, 2016

Still no comment/suggestion/remark of any kind?
(note that the travis error is in regex)

@Manishearth
Copy link
Member

Totally forgot about this, sorry 😄

Will look through.

Manishearth
Manishearth reviewed Mar 6, 2016
View reviewed changes
README.md
@@ -16,6 +16,7 @@ name
[almost_swapped](https://github.com/Manishearth/rust-clippy/wiki#almost_swapped) | warn | `foo = bar; bar = foo` sequence
[approx_constant](https://github.com/Manishearth/rust-clippy/wiki#approx_constant) | warn | the approximate of a known float constant (in `std::f64::consts` or `std::f32::consts`) is found; suggests to use the constant
[bad_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#bad_bit_mask) | warn | expressions of the form `_ & mask == select` that will only ever return `true` or `false` (because in the example `select` containing bits that `mask` doesn't have)
[blacklisted_name](https://github.com/Manishearth/rust-clippy/wiki#blacklisted_name) | warn | usage of a blacklisted name
Copy link
Member

Choose a reason for hiding this comment

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

don't like the name too much, but it's okay.

(placeholder_name instead? But folks may blacklist more than placeholders)

Copy link
Member Author

Choose a reason for hiding this comment

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

But folks may blacklist more than placeholders

Yeah, that’s what I though. I’ll change the short description to “usage of a blacklisted or placeholder name” maybe?

@Manishearth
Copy link
Member

Overall looks good to me. @llogiq, thoughts?

@mcarton mcarton force-pushed the conf branch 2 times, most recently from c607015 to 4998645 Compare March 6, 2016 14:51
@llogiq
Copy link
Contributor

llogiq commented Mar 6, 2016

The failing build appears to be in regex_macros. Will look at the code soon.

@mcarton mcarton force-pushed the conf branch 2 times, most recently from 8390ed1 to 28598ca Compare March 8, 2016 15:45
@mcarton
Copy link
Member Author

mcarton commented Mar 8, 2016

I’ve added the PyLint lint to lint functions with lots of arguments I presented in #694 (what “lots” means being configurable). The default threshold is > 7, which seems (un)reasonable.

Racer, rustful, quickcheck, hyper and clippy have no functions with > 7 arguments. Cargo has two functions with 8 arguments (here and there).

@mcarton
Copy link
Member Author

mcarton commented Mar 8, 2016

By the way @llogiq (and all), do you prefer Clippy.toml (consistent with Cargo.toml) or clippy.toml (consistent with rustfmt.toml)?

@Manishearth
Copy link
Member

lowercase!

@llogiq
Copy link
Contributor

llogiq commented Mar 9, 2016

I also prefer lowercase. It's rust-clippy, not rust-Clippy after all. 😄

@Manishearth
Copy link
Member

Btw, the reason Cargo.toml is uppercase is to be more in line with other build system things like Makefile. We're not a build system thing.

@mcarton
Copy link
Member Author

mcarton commented Mar 9, 2016

Ok then, I yield, lowercase it is 😅

@mcarton mcarton force-pushed the conf branch 2 times, most recently from a6af9b6 to 5609143 Compare March 11, 2016 13:36
@Manishearth
Copy link
Member

Went through it again, lgtm. Thanks!

Manishearth added a commit that referenced this pull request Mar 13, 2016
@Manishearth
Merge pull request #698 from mcarton/conf
eed9baa 
Add a configuration file and a POC `BLACKLISTED_NAME` lint
@Manishearth Manishearth merged commit eed9baa into rust-lang:master Mar 13, 2016
@mcarton mcarton deleted the conf branch March 13, 2016 14:03
@mcarton mcarton mentioned this pull request Mar 14, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mcarton @Manishearth @llogiq