Skip to content

“Style” lint from Pylint and lint configuration #694

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

Closed
3 tasks done
mcarton opened this issue Feb 21, 2016 · 4 comments
Closed
3 tasks done

“Style” lint from Pylint and lint configuration #694

mcarton opened this issue Feb 21, 2016 · 4 comments
Labels
A-lint Area: New lints C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages S-needs-discussion Status: Needs further discussion before merging or work can be started

Comments

@mcarton
Copy link
Member

mcarton commented Feb 21, 2016

Pylint has a few interesting “bad style” lints, eg.:

% cat a.py
def test(a, b, c, d, e, f):
    foo = a+b
    return foo
% pylint a.py
No config file found, using default configuration
[…]
C:  1, 0: Invalid argument name "a" (invalid-name)
[…]
C:  1, 0: Invalid argument name "f" (invalid-name)
[…]
R:  1, 0: Too many arguments (6/5) (too-many-arguments)
C:  2, 4: Black listed name "foo" (blacklisted-name)
[…]

The three of those are interesting I think.

  • invalid-name forbids one-lettered names for parameters or variables (but whitelists i, j, and k, to which I think we should add n; see also Lint non-expressive variable names #644, which suggests linting one-lettered variables if there are more than 3 in the same scope);
  • too-many-arguments lints for “complex” functions with too many parameters, here the default threshold for “too many” is >5;
  • blacklisted-name lints for anything named foo, bar, baz, toto, tutu, or tata (some pylint dev must be French), which you might write “temporarily” eg. for debugging and forget to rename/remove.

The thing is pylint lints are all configurable through command line arguments or in a config file. I think we should have such configuration if we want to implement those lints. The cyclomatic-complexity and probably other lints will also benefit from it.

I suggest a Clippy.toml file, which could contain something like

too-many-arguments-threshold = 7
blacklisted-names = ["toto", "tutu", "tata", "titi"]
cyclomatic-complexity-threshold = 42

maybe in a [configuration] table or something and to allow to use:

#![plugin(clippy(too_many_arguments_threshold="7"))]

What do you think?

@mcarton mcarton added C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages A-lint Area: New lints S-needs-discussion Status: Needs further discussion before merging or work can be started labels Feb 21, 2016
@mcarton mcarton changed the title “Style” lint from Pylint “Style” lint from Pylint and lint configuration Feb 21, 2016
@Manishearth
Copy link
Member

I like the idea. The default too_many_arguments_threshold probably should be larger for us? It's not always easy to struct-ify things and python has named arguments amongst other things to make these things easier to manage

@mcarton
Copy link
Member Author

mcarton commented Feb 21, 2016

That’s what I though too, if we’re going to implement that lint, we should look at real projects to see what unreasonable functions look like.

@killercup
Copy link
Member

I like this! There are a lot of places where clippy can be allowed to make opinionated like these, though, so it might be a good idea to split these things into a separate lint group (or even crate).

The configuration file is a good idea as well. Would it be possible to enable lints/set lint levels there as well? (I can imagine this being difficult).

@ticki
Copy link

ticki commented Feb 26, 2016

I don't like the lint for one letter variables, since (as long as they're not overused or abused) can be a handy tool for temporary variables. I like the idea of restricting the number of single letter variables in the same scope.

@mcarton mcarton closed this as completed Apr 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

No branches or pull requests

4 participants