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

Getting sane #746

Closed
PCManticore opened this issue Dec 17, 2015 · 34 comments
Closed

Getting sane #746

PCManticore opened this issue Dec 17, 2015 · 34 comments
Assignees
Labels
Discussion 🤔 High effort 🏋 Difficult solution or problem to solve Question
Milestone

Comments

@PCManticore
Copy link
Contributor

I want to start a discussion regarding what we want to have in pylint 2.0, from the point of view of the usability of the tool, rather than the static analysis capabilities. There are a lot of folks out there who could use pylint, but they don't use it currently because of various reasons, most of them having
real arguments that I'd like to address with this issue.

When talking about this 2.0, I'm not referring to the previous 2.0 that arised in discussions we had over IRC, with advanced static analysis capabilities, such as control flow graphs, points-to analysis and abstract interpretation. We'll have to call the latter one 3.0 though, since it's going to be another major API breaker. 2.0, in my opinion, should have a main focus of reducing the annoyances that users have with the tool from a UX perspective.

Without further ado, here are the issues I'd like to address with 2.0:

  1. Sane defaults

Pylint's output is too verbose by default. While I agree that most of the messages should
be left as is, we could try to activate the -E flag by default. It should offer the most useful set of errors that users shouldn't have in their project, leaving style checking and other non-critical checks to be activated explicitly by those interested in them.

The reports are not so useful, we could activate "--reports=n" by default. They tend to be useful in some situations, but impedes the normal workflow when working with pylint. The only capability that I'd like to have activated by default is the score, though, since it tends to make the code quality a gamification for beginners.

Remove or downgrade to extensions some checkers, such as too-few-public-methods, too-many-public-methods etc. I'll have to compile a full list for these, but I'm interested in what messages don't seem so useful for the community.

Reevaluate constants for checker's options, such as the maximum number of statements, maximum number of arguments etc. For me, some of them are set too low and I always change them.

  1. Prefer false negatives over false positives

Currently we're doing the opposite thing, we emit a message even if there is a high risk of a false positive to occur. Things changed quite a bit in the last year though regarding this aspect.
For instance, we don't emit no-member if we don't detect all the base classes of the class which owns the attribute we're trying to access.

But we're not there yet, we still have tons of false positives, but this is a topic that's not going to go away so soon, requiring a higher level understanding of Python than we currently have
and more static analysis concepts brought into our need (call graphs, abstract interpretation, flow control understanding, points-to and so much more). What we can do is to try not to emit a message if we're not 100% confident that it's actually a problem. The first example that comes into my
mind is no-member, which we could try not to emit if we can infer multiple values for the same object.
The idea is to keep away false positives, but letting false negatives taking their position. It's much worse to deal with 100 false positives in a project that with 10-15 false negatives (which will eventually be detected correctly, as soon as we'll implement what's missing for them).

So, what do you folks think about this? What else we should target for 2.0 and what would be of interest to you for using pylint more often? The plan is to have this 2.0 out in march-april,
with the main focus for an advanced 3.0 later on this year.

cc @The-Compiler @dmand @ceridwen I'd like your opinion as well on this one.

@RonnyPfannschmidt
Copy link

what i want from a static analysis tool/linter as a normal developer day to day is simply a list of misstakes/errors and silence if that part is fine, an on top of that some kind of editor integration,
that puts red markers on said errors

as nice to have would be finding easy to spot errors quickly (flake8 is good at that)
but stll adding the more tricky ones later (flake8 cant do that)

@sthenault
Copy link
Contributor

I'm fine with this plan. We should keep in mind though that pylint's
exhaustivity make it different from other tools like flake8, and we should
probably focus on that.

On Thu, Dec 17, 2015 at 3:40 PM, Claudiu Popa notifications@github.com
wrote:

I want to start a discussion regarding what we want to have in pylint 20,
from the point of view of the usability of the tool, rather than the static
analysis capabilities There are a lot of folks out there who could use
pylint, but they don't use it currently because of various reasons, most of
them having
real arguments that I'd like to address with this issue

When talking about this 20, I'm not referring to the previous 20 that
arised in discussions we had over IRC, with advanced static analysis
capabilities, such as control flow graphs, points-to analysis and abstract
interpretation We'll have to call the latter one 30 though, since it's
going to be another major API breaker 20, in my opinion, should have a main
focus of reducing the annoyances that users have with the tool from a UX
perspective

Without further ado, here are the issues I'd like to address with 20:

1 Sane defaults

Pylint's output is too verbose by default While I agree that most of the
messages should
be left as is, we could try to activate the -E flag by default It should
offer the most useful set of errors that users shouldn't have in their
project, leaving style checking and other non-critical checks to be
activated explicitly by those interested in them

The reports are not so useful, we could activate "--reports=n" by default
They tend to be useful in some situations, but impedes the normal workflow
when working with pylint The only capability that I'd like to have
activated by default is the score, though, since it tends to make the code
quality a gamification for beginners

Remove or downgrade to extensions some checkers, such as
too-few-public-methods, too-many-public-methods etc I'll have to compile a
full list for these, but I'm interested in what messages don't seem so
useful for the community

Reevaluate constants for checker's options, such as the maximum number of
statements, maximum number of arguments etc For me, some of them are set
too low and I always change them

2 Prefer false negatives over false positives

Currently we're doing the opposite thing, we emit a message even if there
is a high risk of a false positive to occur Things changed quite a bit in
the last year though regarding this aspect
For instance, we don't emit no-member if we don't detect all the base
classes of the class which owns the attribute we're trying to access

But we're not there yet, we still have tons of false positives, but this
is a topic that's not going to go away so soon, requiring a higher level
understanding of Python than we currently have
and more static analysis concepts brought into our need (call graphs,
abstract interpretation, flow control understanding, points-to and so much
more) What we can do is to try not to emit a message if we're not 100%
confident that it's actually a problem The first example that comes into my
mind is no-member, which we could try not to emit if we can infer multiple
values for the same object
The idea is to keep away false positives, but letting false negatives
taking their position It's much worse to deal with 100 false positives in a
project that with 10-15 false negatives (which will eventually be detected
correctly, as soon as we'll implement what's missing for them)

So, what do you folks think about this? What else we should target for 20
and what would be of interest to you for using pylint more often? The plan
is to have this 20 out in march-april,
with the main focus for an advanced 30 later on this year

cc @The-Compiler https://github.com/The-Compiler @dmand
https://github.com/dmand @ceridwen https://github.com/ceridwen I'd
like your opinion as well on this one


Reply to this email directly or view it on GitHub
#746.

Sylvain

@RonnyPfannschmidt
Copy link

an ux and histograms on those statistics would fit a integration in CI tools

the statistics and how they change by what developer are helpfull for team leads,
but in my day to day development its just line noise hiding the real errors

@JonathanWillitts
Copy link

As a (non-contributing) user who wants things to 'just work' out of the box, a "--reports=n" style output would be much more user-friendly (and much less daunting for new users running for the first time). However, I also agree the score should remain included as it's important not just for gamification, but also as a quick sanity check after making code changes (i.e. have I made anything worse).

Losing the I: and perhaps even W: messages by default would make the output cleaner too.

I think getting Python 3.5 support via the pip installed version is important too for new users who have just installed the latest version of Python (especially now 3.5 has been out for a while).

@PCManticore
Copy link
Contributor Author

@JonathanWillitts latest pylint version, 1.5.1, has support for Python 3.5 and it's already released on PyPi for a while now.

@flub
Copy link

flub commented Dec 17, 2015

I agree just defaulting to -E --reports=no would help most people's initial reaction to pylint. On the false positives, if checkers which regularly have false-positives are disabled by default then this is much less of an issue.

@JonathanWillitts
Copy link

@PCManticore my mistake. I've had the 1.4.4 version bookmarked, which I've been regularly checking for 3.5 support.

@The-Compiler
Copy link
Contributor

I've asked about some opinions of people I know complained about this before in #python.

Personally, I'm on the opposite side of things. I like a linter to be as thorough as possible, even if this means it's noisy or shows a few false-positives. For example, in my .eslintrc (another checker for JS, see the website) I turn on a lot of checks they turned off by default.

I think one of pylint's main features over pyflakes is that it's a lot more powerful and detects a lot more, and I'd like to keep it this way.

I can see how being more quiet/conservative helps for users who'd rather not spend a lot of time configuring pylint, though. However, I'd strongly prefer having some checkers or classes of errors disabled by default, definitely not removed. Removing made sense for rules probably nobody uses (star-magic comes to mind), but I'd discourage it for the current checks.

I definitely agree with the point that --report=n should be the default, minus the score. The report takes up so much space for me so I don't even see the issues anymore.


The rules I disable myself are:

  • fixme

I have some FIXME's in my code which are there for a long time (so I don't want them to fail my builds), and usually link to some issue, which is why I disable it.

I'm thinking of getting rid of them (as I have the issues already anyways) and re-enabling this though, so I can actually use fixme for work-in-progress stuff I don't want to forget before cleaning up and pushing.

But I think there are a lot of codebases which use it in the way I described above (i.e. as long-standing todo-lists), where this check just is in the way.

  • global-statement

I think there are some occasions where you can't avoid having global state, and when you do it's much better to do so via global than doing some other way (like building a singleton) - at least it makes it clear something is, in fact, global.
I use global 6 times in some 1000 lines of code, but I think they are very much justified there.

  • locally-disabled
  • suppressed-message
  • file-ignored

Those are just useless meta-noise IMHO, unless one's debugging why some kind of message doesn't show up.

(I'm not sure all of those are enabled by default, I do enable=all)

  • too-many-ancestors
  • too-few-public-methods
  • too-many-public-methods
  • too-many-instance-attributes
  • too-many-lines

I think it's quite difficult to say if something is too complex. I think there are some useful measures in pylint for that, and I think those definitely aren't useful.

I personally use the other too-few/many-* checkers, but I guess they should be disabled by default too.

  • cyclic-import

I want to look at this in detail so I can't really say much, but right now I don't mind cyclic imports as long as they don't raise ImportError, and I think it's difficult to avoid in big projects. In fact, I think avoiding it probably leads to worse code/separation - but as said, no experience with that yet.

  • bad-continuation

This gives me a lot of errors where I clearly find my version a lot easier to read, and some of those might be false-positives. I should probably open an issue for those, but for now I think this is much too agressive.

  • blacklisted-name

I have some stuff called bar (as in a statusbar), and I don't think this is very useful, as this is something I clearly can catch better as a human. There are a lot of bad variable names which can't just be listed on a blacklist.

  • logging-format-interpolation

This is a special case for me, I think this makes sense generally. I log the debug log to RAM however, so the string will always be interpolated, which means I might as well use .format().

  • broad-except
  • bare-except
  • eval-used
  • exec-used

Those might make sense for beginners. For me I always have a good reason when I do this kind of thing, so I want it out of the way.

  • wrong-import-order
  • ungrouped-imports
  • redefined-variable-type

Those are definitely too strict and buggy in the current release. I might want to look at them later, as I see some fixes for the issues I had already.

I also configure some to be less strict:

[BASIC]
function-rgx=[a-z_][a-z0-9_]{2,50}$
const-rgx=[A-Za-z_][A-Za-z0-9_]{0,30}$
method-rgx=[a-z_][A-Za-z0-9_]{2,50}$
attr-rgx=[a-z_][a-z0-9_]{0,30}$
argument-rgx=[a-z_][a-z0-9_]{0,30}$
variable-rgx=[a-z_][a-z0-9_]{0,30}$
docstring-min-length=3

[FORMAT]
max-line-length=79
ignore-long-lines=<?https?://
expected-line-ending-format=LF

[SIMILARITIES]
min-similarity-lines=8

[VARIABLES]
dummy-variables-rgx=_.*

[DESIGN]
max-args=10

@The-Compiler
Copy link
Contributor

@Kwpolska just did run pylint over Nikola and posted the results:

For the (???) ones he doesn't understand what the messages mean exactly - I haven't had time to investigate and look at the code so far.

I think this is a pretty good overview of what's useful, what's not, and maybe a bug (or missing brain module) or two we aren't aware of.

@ceridwen
Copy link

I think I'm in agreement with @PCManticore and @Kwpolska. I have some philosophical differences with how pylint currently works: I want a static analysis tool to find bugs, not complain about formatting or code style issues. For formatting, my preferred solution is something like https://github.com/google/yapf or https://github.com/myint/pyformat, which eliminates the busywork of having to fix code formatting at the same time as finding issues with existing formatting.

I think a lot of what I'll call style checks, to distinguish them from formatting or actual bug checks, are worse than useless for me because they create so much noise in pylint's output. I'm used to, when I use pylint on my own larger projects or other people's projects, wading through hundreds of lines of messages looking for the handful that represent possible bugs and not style issues. The too-many/too-few checkers are the worst offenders, but there are others. Theoretically, I could fix this problem by configuring pylint to ignore all the style messages, but this is a lot of upfront work I've never felt like investing. Is disagreeing with users about intentional choices useful for pylint? When I use map or filter, it's because I disagree with the judgment made by pylint that they shouldn't ever be used. When I use eval or exec, it's because there are no alternatives (or the alternatives are worse). When I use classes without methods as data containers, there's a reason for it. In none of these cases will I change my code whether pylint complains about it or not.

There's a good article about the authors' experiences with commercializing Coverity on the sociology of static analysis tools. They discovered that some users would question even indisputable simple bugs. When pylint flags debatable style issues, particularly when they're deliberate, does that convince the users to change their code or that pylint is wasting their time? Pylint is free so the barrier to entry is lower, but it's not zero. Likewise, I agree with @PCManticore that pylint's false positive rate is too high. Meanwhile, the style checkers inflate this by creating more cases where the users are likely to disagree with the tool. I think style checking has its places for companies/projects that have agreed-upon conventions, but are not so useful in general. They should be opt-in, not opt-out.

@sthenault
Copy link
Contributor

I'm personaly on Florian's side on what I expect from pylint. And I think
his way of listing messages that he think could/should be removed is
probably the most constructive and efficient way to reach a consensus.

Part of this has already been discussed before, and I think we already
agree on two things :

  • reports should be disabled by default
  • though the global evaluation should be kept as a key differencing factor
    of pylint

I also would like to recall that pylint's philosophy has always been that
one should not expect to fix all messages, because some of them are only
pointer to things that may be undesired / done differently, while it's of
course perfectly fine for the developper to keep it as is. That's also why
we have such a sensible control of messages. Also, I'm not really in favor
of having a lot of things disabled by default, because I feel like most
people won't ever take a look at them. If we go that way I would rather
like to see a registry of pylint plugins, not shiped with the core. But
that's development, documentation and communication work.

All that is of course not contradictory with the fact that some messages
could be considered useless, that default values may be too restrictive, or
that the less false positive the better. Below are my preferences about
this.

Message that must be removed (I've always seen a consensus on this):

  • too-many-ancestors
  • too-few-public-methods
  • too-many-public-methods
  • too-many-instance-attributes

Message that could be removed:

  • too-many-lines
  • too-many-arguments
  • too-many-locals
  • fixme
  • blacklisted-name

Messages that should be disabled by default

  • locally-disabled
  • locally-enabled
  • suppressed-message
  • file-ignored

Also I'm fine with Claudiu's proposal to have higher values for remaining
too-many-* messages or alike.

A side note to finish: we (logilab) have recently added new messages, some
of which seem indeed problematic yet as Florian noticed
(wrong-import-order, ungrouped-imports, redefined-variable-type). I
personnaly think that those will become nice to have messages once most
problematic issues with them will be sorted out, and so they are worth the
effort.

FYI, you'll find attached a list of all current pylint messages.

On Fri, Dec 18, 2015 at 4:44 AM, ceridwen notifications@github.com wrote:

I think I'm in agreement with @PCManticore
https://github.com/PCManticore and @Kwpolska
https://github.com/Kwpolska. I have some philosophical differences with
how pylint currently works: I want a static analysis tool to find bugs, not
complain about formatting or code style issues. For formatting, my
preferred solution is something like https://github.com/google/yapf or
https://github.com/myint/pyformat, which eliminates the busywork of
having to fix code formatting at the same time as finding issues with
existing formatting.

I think a lot of what I'll call style checks, to distinguish them from
formatting or actual bug checks, are worse than useless for me because they
create so much noise in pylint's output. I'm used to, when I use pylint on
my own larger projects or other people's projects, wading through hundreds
of lines of messages looking for the handful that represent possible bugs
and not style issues. The too-many/too-few checkers are the worst
offenders, but there are others. Theoretically, I could fix this problem by
configuring pylint to ignore all the style messages, but this is a lot of
upfront work I've never felt like investing. Is disagreeing with users
about intentional choices useful for pylint? When I use map or filter, it's
because I disagree with the judgment made by pylint that they shouldn't
ever be used. When I use eval or exec, it's because there are no
alternatives (or the alternatives are worse). When I use classes without
methods as data containers, there's a reason for it. In none of these cases
will I change my code whether pylint complains about it or not.

There's a good article about the authors' experiences with
commercializing Coverity
http://cacm.acm.org/magazines/2010/2/69354-a-few-billion-lines-of-code-later/fulltext
on the sociology of static analysis tools. They discovered that some users
would question even indisputable simple bugs. When pylint flags debatable
style issues, particularly when they're deliberate, does that convince the
users to change their code or that pylint is wasting their time? Pylint is
free so the barrier to entry is lower, but it's not zero. Likewise, I agree
with @PCManticore https://github.com/PCManticore that pylint's false
positive rate is too high. Meanwhile, the style checkers inflate this by
creating more cases where the users are likely to disagree with the tool. I
think style checking has its places for companies/projects that have
agreed-upon conventions, but are not so useful in general. They should be
opt-in, not opt-out.


Reply to this email directly or view it on GitHub
#746 (comment).

Sylvain

blacklisted-name
invalid-name
missing-docstring
empty-docstring
unneeded-not
singleton-comparison
misplaced-comparison-constant
unidiomatic-typecheck
consider-using-enumerate
bad-classmethod-argument
bad-mcs-method-argument
bad-mcs-classmethod-argument
line-too-long
too-many-lines
trailing-whitespace
missing-final-newline
multiple-statements
superfluous-parens
bad-whitespace
mixed-line-endings
unexpected-line-ending-format
bad-continuation
wrong-spelling-in-comment
wrong-spelling-in-docstring
invalid-characters-in-docstring
multiple-imports
wrong-import-order
ungrouped-imports
wrong-import-position
old-style-class
syntax-error
unrecognized-inline-option
bad-option-value
init-is-generator
return-in-init
function-redefined
not-in-loop
return-outside-function
yield-outside-function
return-arg-in-generator
nonexistent-operator
duplicate-argument-name
abstract-class-instantiated
bad-reversed-sequence
continue-in-finally
method-hidden
access-member-before-definition
no-method-argument
no-self-argument
invalid-slots-object
assigning-non-slot
invalid-slots
inherit-non-class
inconsistent-mro
duplicate-bases
non-iterator-returned
unexpected-special-method-signature
import-error
used-before-assignment
undefined-variable
undefined-all-variable
invalid-all-object
no-name-in-module
unbalanced-tuple-unpacking
unpacking-non-sequence
bad-except-order
raising-bad-type
misplaced-bare-raise
raising-non-exception
notimplemented-raised
catching-non-exception
slots-on-old-class
super-on-old-class
bad-super-call
missing-super-argument
no-member
not-callable
assignment-from-no-return
no-value-for-parameter
too-many-function-args
unexpected-keyword-arg
redundant-keyword-arg
invalid-sequence-index
invalid-slice-index
assignment-from-none
not-context-manager
invalid-unary-operand-type
unsupported-binary-operation
repeated-keyword
not-an-iterable
not-a-mapping
unsupported-membership-test
unsubscriptable-object
logging-unsupported-format
logging-format-truncated
logging-too-many-args
logging-too-few-args
bad-format-character
truncated-format-string
mixed-format-string
format-needs-mapping
missing-format-string-key
too-many-format-args
too-few-format-args
bad-str-strip-call
print-statement
parameter-unpacking
unpacking-in-except
old-raise-syntax
backtick
long-suffix
old-ne-operator
old-octal-literal
import-star-module-level
fatal
astroid-error
parse-error
method-check-failed
raw-checker-failed
bad-inline-option
locally-disabled
locally-enabled
file-ignored
suppressed-message
useless-suppression
deprecated-pragma
too-many-nested-blocks
simplifiable-if-statement
no-self-use
no-classmethod-decorator
no-staticmethod-decorator
redefined-variable-type
cyclic-import
duplicate-code
too-many-ancestors
too-many-instance-attributes
too-few-public-methods
too-many-public-methods
too-many-return-statements
too-many-branches
too-many-arguments
too-many-locals
too-many-statements
too-many-boolean-expressions
unreachable
dangerous-default-value
pointless-statement
pointless-string-statement
expression-not-assigned
unnecessary-pass
unnecessary-lambda
duplicate-key
deprecated-lambda
useless-else-on-loop
exec-used
eval-used
confusing-with-statement
using-constant-test
bad-builtin
lost-exception
assert-on-tuple
attribute-defined-outside-init
bad-staticmethod-argument
protected-access
arguments-differ
signature-differs
abstract-method
super-init-not-called
no-init
non-parent-init-called
unnecessary-semicolon
bad-indentation
mixed-indentation
lowercase-l-suffix
wildcard-import
deprecated-module
relative-import
reimported
import-self
misplaced-future
fixme
invalid-encoded-data
global-variable-undefined
global-variable-not-assigned
global-statement
global-at-module-level
unused-import
unused-variable
unused-argument
unused-wildcard-import
redefined-outer-name
redefined-builtin
redefine-in-handler
undefined-loop-variable
cell-var-from-loop
bare-except
broad-except
duplicate-except
nonstandard-exception
binary-op-exception
property-on-old-class
logging-not-lazy
logging-format-interpolation
bad-format-string-key
unused-format-string-key
bad-format-string
missing-format-argument-key
unused-format-string-argument
format-combined-specification
missing-format-attribute
invalid-format-index
anomalous-backslash-in-string
anomalous-unicode-escape-in-string
bad-open-mode
boolean-datetime
redundant-unittest-assert
deprecated-method
apply-builtin
basestring-builtin
buffer-builtin
cmp-builtin
coerce-builtin
execfile-builtin
file-builtin
long-builtin
raw_input-builtin
reduce-builtin
standarderror-builtin
unicode-builtin
xrange-builtin
coerce-method
delslice-method
getslice-method
setslice-method
no-absolute-import
old-division
dict-iter-method
dict-view-method
next-method-called
metaclass-assignment
indexing-exception
raising-string
reload-builtin
oct-method
hex-method
nonzero-method
cmp-method
input-builtin
round-builtin
intern-builtin
unichr-builtin
map-builtin-not-iterating
zip-builtin-not-iterating
range-builtin-not-iterating
filter-builtin-not-iterating
using-cmp-argument

@The-Compiler
Copy link
Contributor

Also, I'm not really in favor of having a lot of things disabled by default, because I feel like most people won't ever take a look at them.

I fear the same, but I think this can be solved with a good "getting started" documentation.

If we go that way I would rather like to see a registry of pylint plugins, not shiped with the core. But that's development, documentation and communication work.

Some useful plugin system would indeed be a nice thing. Ideally something based on setuptools entry points, so you could simply install pylint-style, pylint-pedantic, etc. etc. via pip 😉

Message that could be removed

As mentioned I'd prefer to see stuff disabled by default (or moved to plugins) than removed completely. Some of those surely are useful for some poeple.

A side note to finish: we (logilab) have recently added new messages, some of which seem indeed problematic yet as Florian noticed (wrong-import-order, ungrouped-imports, redefined-variable-type). I personnaly think that those will become nice to have messages once most problematic issues with them will be sorted out, and so they are worth the effort.

I agree with that - I can see their value if they work properly.

@RonnyPfannschmidt
Copy link

what might be a nice path to slowly adopting more rules/configuration could be to notify users of the next tier of messages once they cleared the basic issues

so one gets bugged a bit about enabling more checks over time without getting pressed into it y failing builds

@flub
Copy link

flub commented Dec 18, 2015

A lot of these things are rather subjective and I think the trick is to make them opt-in not remove them as I've seen argued here for some. I use pylint in a team of mixed experienced and I do find it helpful that pylint will complain that there are too many statements, that a line is too long, that the import order is wrong (love this new one!) etc etc. These are described in some posts above as bullshit or must be removed but I often find that the overall design of the code improves when you try and re-architect the problem avoiding those. The great thing is all of these can be configured, the number of statements, the number of lines, how long lines are etc etc.

So I'll reiterate, the trick is to make these more subjective checks that will require creating your own pylintrc to configure them for your project's preferences opt-in, not to remove them.

@nbastin
Copy link
Contributor

nbastin commented Dec 21, 2015

I definitely want a tool that can evaluate and error on a style, but pylint needs to be far more configurable to be that tool (PEP8 is definitely not the style I want to enforce). You can write extensions to do this now, but it's not the smoothest workflow, plus it can get inefficient if you're duplicating visits a lot (I don't have a great idea how to fix this off the top of my head, but it needs some kind of pre-processor probably).

What bothers me the most is checks that are added that don't seem to be designed at all - unneeded-not is a classic example of something that if it had been thought through and designed from the start with a truly complete set of tests it never would have had the problem of reporting on __ne__ implementations. redefined-variable-type also seems like something that bit off far more than it could chew. Any time a check is going to be added it should be actually designed on paper and reviewed before it gets implemented, or at least before it gets added to the default set of checkers.

@The-Compiler
Copy link
Contributor

@nbastin Do you have an example of something you'd like to configure but can't currently?

As for unneeded-not and redefined-variable-type, I think by now everyone agrees it was a mistake to merge them quickly before the release. I disagree designing stuff on paper would've changed anything about that, though.

@nbastin
Copy link
Contributor

nbastin commented Dec 24, 2015

@The-Compiler I haven't had time to put together a complete list (by far), but a few examples would be:

We want a whitespace between the name and open-parnes in a function/method definition, but not in its' use:

 def method (self, some_int):
  ...

vs.

method(7)

Also more configurable parameters for things like too-many-arguments, to allow for a distinction between the count of non-default arguments versus those with default values.

I tried to dig up examples of user-configurable stuff from the C++ tool we use, but the license agreement won't let me share them.. :-/

@PCManticore
Copy link
Contributor Author

PCManticore commented Dec 24, 2015

Thank you all for your responses, they are real insightful for how you're using pylint and what you're expecting from it.

I'll try to summarize the next course of action in this comment.

  • disable reports by default, enable the score by default
  • disable the information category by default
  • increase option values for the some checkers (too many arguments etc.)
  • regarding false positives, abort a check when multiple values with different types are inferred. Add a --confidence flag, which will control this behaviour. By default the --confidence should have a value for preferring false negatives, but a simple pylint --confidence=full should trigger the current behaviour.

On the next item, the opinions seems to be split. Some of you are using the tool without expecting upfront for some messages to be disabled by default, other would like for categories such as conventions to be disabled by default. I'm not sure where to fall here, but here's an idea that's a bit inspired by Ronny's idea of tier messages.

We could split the possible categories into four major classes: core pylint messages (violation of the type system, accessing a member which doesn't exist etc), pedantic (the current warnings. They can be actual error or things that we can't figure out), refactoring (things that could be refactored in order to be easier to understand), style (style checkers, that are not necessarily vital in a project)

What I want to do with these coarse categories is to provide tier levels, that needs to be solved first by the user before going to the next one. By default the core is enabled.

$ pylint myproject
# core checkers enabled
10/10 - Congrats, you're clean on a core. You might try with "--pedantic" now.

$ pylint myproject --pedantic
# pedantic checkers enabled.
10/10 - Congrats, you're clean on pedantic. You might try with --refactoring now

$ pylint myproject --refactoring
# refactoring checkers enabled
10/10 - Congrats, you're clean on refactoring. Last up, try with --style now.

$ pylint myproject --style
10/10 - Now you're pylint clean.

Now the project is pylint clean. At each step, the user can decide to ignore some messages per each stage, without encountering them anymore in the next runs.

What do you think about this last step? Apart of this, I think we can already start doing the rest of the points we mentioned.

@PCManticore PCManticore self-assigned this Dec 24, 2015
@PCManticore PCManticore added the Blocker 🙅 Blocks the next release label Dec 24, 2015
@Kwpolska
Copy link

There should also be a way to run all four suites at once:

$ pylint myproject --everything
Core:         10/10
Pedantic:     10/10
Refactoring:  10/10
Style:        10/10
AGGREGATE:    10/10 -- congrats, you're pylint clean.

@flub
Copy link

flub commented Dec 28, 2015

Hi Claudiu,

Seems like a great approach.

One minor thing which you might want to consider is to use something like
--level=core|pedantic|refactoring|style instead of the different options to
make it more obvious they are layered on top of each other.

Regards,
Floris

On 24 December 2015 at 17:47, Claudiu Popa notifications@github.com wrote:

Thank you all for your responses, they are real insightful for how you're
using pylint and what you're expecting from it.

I'll try to summarize the next course of action in this comment.

  • disable reports by default, enable the score by default
  • disable the information category by default
  • increase option values for the some checkers (too many arguments
    etc.)
  • regarding false positives, abort a check when multiple values with
    different types are inferred. Add a --confidence flag, which will control
    this behaviour. By default the --confidence should have a value for
    preferring false negatives, but a simple pylint --confidence=full
    should trigger the current behaviour.

On the next item, the opinions seems to be split. Some of you are using
the tool without expecting upfront for some messages to be disabled by
default, other would like for categories such as conventions to be disabled
by default. I'm not sure where to fall here, but here's an idea that's a
bit inspired by Ronny's idea of tier messages.

We could split the possible categories into four major classes: core
pylint messages (violation of the type system, accessing a member which
doesn't exist etc), pedantic (the current warnings. They can be actual
error or things that we can't figure out), refactoring (things that could
be refactored in order to be easier to understand), style (style checkers,
that are not necessarily vital in a project)

What I want to do with these coarse categories is to provide tier levels,
that needs to be solved first by the user before going to the next one. By
default the core is enabled.

$ pylint myproject

core checkers enabled

10/10 - Congrats, you're clean on a core. You might try with "--pedantic" now.

$ pylint myproject --pedantic

pedantic checkers enabled.

10/10 - Congrats, you're clean on pedantic. You might try with --refactoring now

$ pylint myproject --refactoring

refactoring checkers enabled

10/10 - Congrats, you're clean on refactoring. Last up, try with --style now.

$ pylint myproject --style
10/10 - Now you're pylint clean.

Now the project is pylint clean. At each step, the user can decide to
ignore some messages per each stage, without encountering them anymore in
the next runs.

What do you think about this last step? Apart of this, I think we can
already start doing the rest of the points we mentioned.


Reply to this email directly or view it on GitHub
#746 (comment).

@The-Compiler
Copy link
Contributor

I like that approach as well, and I agree with what @flub said.

PCManticore added a commit that referenced this issue Jan 3, 2016
This is a step towards making pylint more sane, as per the discussion from issue #746.
PCManticore added a commit that referenced this issue Jan 3, 2016
As per discussion from issue #746, the reports were disabled by
default in order to simplify the interaction between the
tool and the users. The score is still shown by default,
as a way of closely measuring when it increases or decreases
due to changes brought to the code.
The patch introduces a new command line flag, "--score" or its
shorthand version, "-s", which controls if the score is shown or not.
By default, it's set to true.
@PCManticore PCManticore added this to the 2.0 milestone Jun 5, 2016
@pradyunsg
Copy link

@PCManticore You might want to remove "in 2.0" from the issue title.

@PCManticore PCManticore changed the title Getting sane in 2.0 Getting sane Oct 12, 2018
@gilmrjc
Copy link

gilmrjc commented Oct 30, 2018

I think the same as here:

Personally, I'm on the opposite side of things. I like a linter to be as thorough as possible, even if this means it's noisy or shows a few false-positives. For example, in my .eslintrc (another checker for JS, see the website) I turn on a lot of checks they turned off by default.

I think one of pylint's main features over pyflakes is that it's a lot more powerful and detects a lot more, and I'd like to keep it this way.

I can see how being more quiet/conservative helps for users who'd rather not spend a lot of time configuring pylint, though. However, I'd strongly prefer having some checkers or classes of errors disabled by default, definitely not removed. Removing made sense for rules probably nobody uses (star-magic comes to mind), but I'd discourage it for the current checks.

Going forward with the example, why not doing what eslint does? To improve the configuration experience from users, eslint provides the ability to use "presets". These are packages that active the rules according to the preset. The most common are eslint-airbnb and eslint-standard. You can even combine them to establish the project's rules.

PyLint already provides a way to implement this functionality thanks to BaseChecker.config. A reasonable solution would be to create a PyLint preset package with multiple leves like "pylint_presets.core", "pylint_presets.base", "pylint_presets.strongly-recommended", "pylint_presets.recommended", etc.

With these options a user can enable the core preset showing only errors, or the base preset showing sane basic defaults, or the recommended with a set of rules to enforce best practices. And even the user can use a custom preset with their rules of preference.

This system could work with an autodiscovery system like flake8 does with plugins, and the users add what they feel is better for them without worrying for the configuration, keeping the ability to override the rules they want if the preset isn't exactly what they need.

@AWhetter
Copy link
Contributor

This is an old issue, but there's a lot of important points. So I'm going to dump some more thoughts:

"pylint is opinionated":
I've heard this a lot and never got a clear answer on what it means. If anyone has this opinion, please share why you think this is.
I've interpreted it as "pylint isn't configurable". I've tried to take this to heart when adding new checks or editing existing ones, but we have a way to go with the existing ones. For example, too-few-public-methods is valid in some cases and theoretically you could configure when it's fine rather than needing to either silence the message with pragmas or turn the message off completely.
I think which messages we have turned on by default plays a role in this. It's been discussed already, to the point where I think we can action on it. Having an audit of which checks are currently enabled and which should be seems like a good next step to me?

False positives:
I totally agree with the sentiments in the issue description. We get issues in all the time about no-member messages on C modules, for example. I'm not sure what the solution is for it yet, but it's definitely an issue.

Configuration:
The config system is not my favourite part of pylint. The generated pylintrc files have so much stuff in them and it's daunting. It's better now that we allow reading from pyproject.toml files, but it always bothered me that the options are separated into different sections. It makes it more daunting to get started with changing the configuration file. The sections seem arbitrary.
Pragmas are also a bit ugly. To the point where we suggested combining with #noqa in (#2493). Even though we can't combine with #noqa, I think we can still improve on the length and simplicity of # pylint: disable=line-too-long.
The code for configuration is challenging to work with as well. To the point where it prevented the per_dir_config project (#618) from being even close to easy. We've spoken before about what would a theoretical pylint rewrite look like. I think about it often because I think it would be a good thing. We struggle to find maintainers, and I think it's partly because the codebase is so difficult to get started with. But I don't think we would need to go from scratch with a rewrite. I would keep the contents of the checkers and their logic the same, but everything around that like how to launch the checkers, reading config, reporting, loading plugins, etc I would rewrite. This would let us fix things like the brokenness of parallel checking as well. Something this drastic is obviously only possible with a major version bump. Maybe it's a pipe dream. We struggle with development as it is. But hey, a guy can dream.

Plugins:
I agree with what's been said. I would even split things like the score out into a plugin. Reports could be a plugin. We have a huge burden as maintainers and it's partly because we support something so big. Splitting it up into plugins might allow for more external development outside the core project. It's the direction that flake8 has and it works well.

@AWhetter
Copy link
Contributor

Style:
The more time goes on, the more I think we could remove style checking altogether. I've always preferred flake8 for style checking. It's a bit more configurable and has a wider range of checks.
But now that things like black exist, they generally do a better job of styling than we do.
Plus the code is a nightmare!
Perhaps it's another set of plugins we can think about splitting out.

@The-Compiler
Copy link
Contributor

I've heard this a lot and never got a clear answer on what it means. If anyone has this opinion, please share why you think this is.

I might repeat a bit of what I said in #746 (comment) already, but it's been a while!

I guess it depends on how you approach using a linter. My personal approach is "give me the firehose of everything you can possibly find, and I'm going to spend the time turning off everything I don't agree with". As such, I've been very happy with pylint, as it's much more powerful than flake8 if you spend the time needed to configure it.

However, most people I've spoken to consider the default configuration way too noisy - they'd rather have a linter which they can start using right away, without any configuration needed, and get sensible results out-of-the-box.

So, personally, I'd still turn on everything and filter out rules individually - but many others would probably like something a bit more quiet (but still useful).

See e.g. how eslint has a list of rules and only enables "rules that report common problems". They also only change that set with a major release, so people can safely upgrade the linter without having new "problems" to take care of.

However, playing devil's advocate, I'm wondering how useful pylint really is at that point compared to flake8/black (for style things) plus mypy (with --check-untyped-defs). I've been thinking about dropping pylint entirely for my projects (since it's much slower than mypy/flake8/black), but I'm not sure how much benefit it still brings me over other tools.

@AWhetter
Copy link
Contributor

I was at PyCascades last weekend and got a few more bits of feedback for us to think on.

Default messages: I think we already agree on this so I won't say much, but there was agreement that changing the default messages would be a good thing. Some people turn everything off when they start using pylint and build up a list of enabled messages themselves.

Working with flake8: pylint is used so frequently with flake8 that it would be useful to have documentation on how to use both together. So noting down things like what messages are duplicated between both, and any messages that pylint and flake8 have disagreements on. There's probably something to be said about style checks as well.

Style: Continuing from the last point, flake8 definitely seemed to be the preferred method of style checking over pylint. People said they wouldn't miss the style checks if we do decide to either remove them or move them to a plugin, because many were using flake8 for that.

Ease of use: There was a comment that junior developers struggle to get started with pylint, and part of that is because the messages generally only tell you what's wrong and not how to fix it.
(Where I was last we had integration tests on our linters. Tests would check for lint errors and have an equivalent passing test. We then used these in the documentation for error code to link to examples. All of this was generated automatically. We could do something similar.)
The emphasis on using message names rather than message codes is a big plus over flake8.

@DanielNoord
Copy link
Collaborator

DanielNoord commented Jan 4, 2022

To get this thread some traction again I think we need to redefine what needs to be done to close this. To recap what I believe are the main points and what still needs to be done.

  • Reports have been turned off by default
  • Code style has been moved to an extension and has therefore become opt-in like most wanted to
  • We need better defaults > This is being tracked in Disable certain messages by default #3512 and is therefore not really a blocker for this issue
  • We need to "tier" or split certain message categories > I think this is the remaining issue in this issue.

Focusing on that last issue:
I totally agree with what the @ghost said and don't think we should add too many options to the command-line again. Every option we add also gets added to the default generated pylintrc, which, as has been noted in this topic before, is already very large and daunting.
A much better option would be the presets discussed by @gilmrjc. I have also used those before on eslint and they work really well. It also allows organisations and individuals to quickly share their preset of messages with others.
I'd like the opinion of others on this as well but for now I would propose to change this issue name to "Allow configuring message tiers or presets" and see what the consensus on this last issue will be.

@Pierre-Sassoulas Pierre-Sassoulas changed the title Getting sane Allow configuring message tiers or presets Jan 4, 2022
@Pierre-Sassoulas
Copy link
Member

I think we also have:

@Pierre-Sassoulas Pierre-Sassoulas added High effort 🏋 Difficult solution or problem to solve and removed Blocker 🙅 Blocks the next release labels Jul 4, 2022
@Pierre-Sassoulas Pierre-Sassoulas changed the title Allow configuring message tiers or presets Getting sane Jul 4, 2022
@Pierre-Sassoulas
Copy link
Member

Closing in favor of #7120 and #7121.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion 🤔 High effort 🏋 Difficult solution or problem to solve Question
Projects
None yet
Development

No branches or pull requests