-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Comments
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, as nice to have would be finding easy to spot errors quickly (flake8 is good at that) |
I'm fine with this plan. We should keep in mind though that pylint's On Thu, Dec 17, 2015 at 3:40 PM, Claudiu Popa [email protected]
Sylvain |
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, |
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). |
@JonathanWillitts latest pylint version, 1.5.1, has support for Python 3.5 and it's already released on PyPi for a while now. |
I agree just defaulting to |
@PCManticore my mistake. I've had the 1.4.4 version bookmarked, which I've been regularly checking for 3.5 support. |
I've asked about some opinions of people I know complained about this before in 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 ( I definitely agree with the point that The rules I disable myself are:
I have some 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 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.
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
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
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
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
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.
I have some stuff called
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
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.
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:
|
@Kwpolska just did run pylint over Nikola and posted the results: For the 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. |
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. |
I'm personaly on Florian's side on what I expect from pylint. And I think Part of this has already been discussed before, and I think we already
I also would like to recall that pylint's philosophy has always been that All that is of course not contradictory with the fact that some messages Message that must be removed (I've always seen a consensus on this):
Message that could be removed:
Messages that should be disabled by default
Also I'm fine with Claudiu's proposal to have higher values for remaining A side note to finish: we (logilab) have recently added new messages, some FYI, you'll find attached a list of all current pylint messages. On Fri, Dec 18, 2015 at 4:44 AM, ceridwen [email protected] wrote:
Sylvain
|
I fear the same, but I think this can be solved with a good "getting started" documentation.
Some useful plugin system would indeed be a nice thing. Ideally something based on setuptools entry points, so you could simply install
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.
I agree with that - I can see their value if they work properly. |
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 |
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. |
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 - |
@nbastin Do you have an example of something you'd like to configure but can't currently? As for |
@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:
vs.
Also more configurable parameters for things like 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.. :-/ |
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.
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.
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. |
There should also be a way to run all four suites at once:
|
Hi Claudiu, Seems like a great approach. One minor thing which you might want to consider is to use something like Regards, On 24 December 2015 at 17:47, Claudiu Popa [email protected] wrote:
|
I like that approach as well, and I agree with what @flub said. |
This is a step towards making pylint more sane, as per the discussion from issue #746.
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 You might want to remove "in 2.0" from the issue title. |
I think the same as here:
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 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. |
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": False positives: Configuration: Plugins: |
Style: |
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 |
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. |
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.
Focusing on that last issue: |
I think we also have:
|
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:
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.
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.
The text was updated successfully, but these errors were encountered: