Thoughts on making pylint non-blocking

[Rick Byers]

Hey,

We're considering whether to land this CL making pylint non-blocking. 

Looking over the disables in our codebase there's a pretty broad set. line-too-long is quite common though, and at least for this I think we'd want to be consistent in how we handle py and C++ code. I think clang format being non-blocking has been working alright, hasn't it? Most people just follow 'git cl format', but occasionally there's a good reason to ignore it's advice and just ignoring it is easier and cleaner. I certainly would not support having to put an explicit linter disable annotation on long C++ lines. So in that case making pylint non-blocking sounds right to me.

But what about the other cases? I looked at some, especially bare-except/broad-except cases and don't personally have a strong opinion.

Thanks,

   Rick

[Dirk Pranke]

In general, I wouldn't be a fan of this. I have not found in my experience that pylint is particularly noisy once you have whatever settings you need in the config file, and I don't find pylint messages to be false positives. Perhaps we just don't have the right settings in the config file? Or perhaps we're not using multiple config files for different parts of the codebase? We definitely have different standards for Blink and non-Blink code, for example. But I would've thought we had that in place already.

In general, I'm not a fan of warnings; I tend to think things should either be errors or silent.

I would've also thought that c++ warnings were also blocking, though. I do see value in being consistent across languages.

Certainly I agree w/ erikchen@'s comment on the CL, it would be good to see examples of what you think the problems are.

-- Dirk

--
You received this message because you are subscribed to the Google Groups "python" group.
To unsubscribe from this group and stop receiving emails from it, send an email to python+un...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/python/CAFUtAY-55Bv6momCqbHQvth4vJ%2BnO2OsoOG0vOz_KH%2BBsO-joQ%40mail.gmail.com.

[Mohamed Heikal]

I can see Dirk's point that this will make the linter useless. I think the main issue is that the linter is too strict about things that do not matter. But I guess we decide what rules to apply. So, how about instead of making pylint non-blocking we disable most formatting related lint errors, leave the formatting to `git cl format` and reviewer comments. The most egregious check is "long line" since we already have a different presubmit for that which doesn't block.

[Dirk Pranke]

Do you have examples where you need extra long lines and you're getting complaints from pylint when you shouldn't be? 

Personally I'm a fan of enforced line length limits as long as you have the correct support for exceptions (e.g., a long URL), but if there's a general consensus to not always enforce that, I can be convinced :).

Also, I'm not sure what you mean by "non-blocking"? Keeping the change from being uploaded? Keeping the change from landing? Both? I don't actually know what we do today, either for Python or C++.

-- Dirk 

[Alexei Svitkine]

One thing I ran into (and haven't had more time to pursue) is that our current Pylint seems to be both out of date and also not always? honoring "pylint: disable" statements.

I have (a very low priority) CL blocked on that: https://chromium-review.googlesource.com/c/chromium/src/+/5743523

I'm using Python type annotations, but our Pylint isn't able to understand types like list[str] and pylint comments seem insufficient to disable this. So this gets blocked by cq (red chips on the CL).

(I can remove the type annotations as a workaround, but it seems not ideal.)

It seems the issue is we're stuck on an old pylint version, but I'm not sure whose responsibility it is to upgrade it - I guess maybe there's no POC and it's up to whoever wants it (and on my side it hasn't been high priority to unblock that CL).

I did leave a comment on crbug.com/40850532 which tracked the last time it was updated, but no one chimed in.

So if we're not actively maintaining a current pylint version, disabling seems reasonable?

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/python/CAEoffTDHpwKK4O2hmUNhE5Y-LpMpDnnHtqQLGj9HNbVv%2B%2Bm4Rg%40mail.gmail.com.

[Dirk Pranke]

As you say, I don't think there's anyone actively responsible for updating pylint. While our version is only a year old, I would expect there to have been a bunch of bug fixes since 2.17. And, removing type annotations is definitely not ideal (although in your case I'd guess you could probably use typing.List[str] instead of list[str]).

More importantly, now that I've dug into things a bit, it's not actually clear to me how much code is being run under pylint at all. 

Given the above, I guess I'm okay with disabling things until things are in a more maintained shape. Can we work together to figure out which things to disable? I'd like to be clear about which things we definitely don't want to run vs. things where there might be differences of opinion vs. things that are just buggy.

I'll also see if I can figure out a plan to make a current pylint version available.

-- Dirk

[Mike Frysinger]

there's a bunch of threads in here.  i'll try to go through them.

non-blocking:

as Dirk said, it's either errors that block & you see, or it's quiet.  if it's not a blocker, people will ignore it, and new warnings will be introduced.  for people who want to DTRT and not write dirty code, it gets harder and harder to figure out from the noise what they added, and what was already there.  eventually, even the well meaning people give up and declare bankruptcy.  i've seen this play out in CrOS projects time & again.

formatting:

pylint has largely gotten out of this business.  many of its formatting-oriented lint checks have been out right deleted.  any ones left would be best disabled in our default configs.  if people want to enforce formatting, then turn on yapf by adding .style.yapf to your tree.  would be nice if we could adopt a src/.style.yapf here.

pylint ownership for new versions:

there is no one here who is responsible, and i don't think there ever has been.  it's trivial to add a new version in parallel to existing ones.  so if you want a new X.Y version, add it.  that's basically how it's been handled.

default pylint version:

since no one owns it, no one is responsible for updating the default version.  if you try to change the default version, then you're responsible for fixing any new lint warnings.  very few teams will help out here -- more likely the upgrade will get reverted, and then you have to fix & reland.  that's why we've settled on each subdir that opts in to pylint locking the version, and not really having a tree-wide default.  see input_api.canned_checks.GetPylint(..., version='...') in PRESUBMIT.py files.

disabling diagnostics by default:

the default list in ~/depot_tools/pylintrc of disabled diagnostics is indeed huge.  this is a direct result of the issues i noted above -- the person who tries to update the default version is responsible for cleaning up all the new warnings, and when that amounts to hundreds if not thousands across Chromium projects, it's easier to disable the new diagnostics and force projects to opt-in after they clean things.  some do, but many don't.

being too strict:

this feels like a fairly subjective area.  one person's "too strict" is another person's "too loose".  it's up to each team/area to set the knobs that they want, but once set, they should be followed.  i won't try to defend every diagnostic because there are some that def veer into style-vs-substance, and some that don't fully account for full context, but i will say that it's not uncommon to get pushback from devs who don't fully appreciate the nuances of Python.  bare-except/broad-except are a good example of this imo as people don't realize that these can easily suck up things like SIGINT/Ctrl+C or syntax errors.

pylint out of date / disable statements not working:

pylint has supported "pylint: disable=" for basically ever.  what you're asking about specifically is "disable-next" which is new to pylint 2.10 iiuc.  so the issue isn't that pylint in general is out of date, it's that the subtree you're trying to make changes in is using a pylint version that is too old.  you need to update that if you want to use newer syntax.  look at testing/PRESUBMIT.py which pins to pylint-2.7.  g'luck updating all of testing/ to not introduce new warnings.

that said, pylint is flagging that as an error, not warning.  i'm not sure you can disable syntax errors.  then again, i'm not sure you should ... see next section.

type annotations:

"list[str]" syntax is new to Python 3.9.  older versions need to use "from typing import List" and "List[str]" (which also works with newer versions).  the pylint-2.6 & pylint-2.7 versions in depot_tools are using python 3.8 while pylint-2.17 is using python 3.11.  but this also strongly implies that whatever python code you're writing is implicitly requiring python 3.9+, so if you have scripts using vpython3 pinned to 3.8, you're breaking them, and presubmit is right to block you.

how common is pylint:

cs/ says that a lot of PRESUBMIT.py files are running pylint.  not sure how to quantify that into % of python files in the tree though.

https://source.chromium.org/search?q=f:PRESUBMIT.py%20(RunPylint%7CGetPylint)&sq=&ss=chromium%2Fchromium%2Fsrc

-mike

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/python/CAEoffTBD1GEyyBjaZ6ROR-vS12ROdJbTDkzCQXKRPn8Goya6rQ%40mail.gmail.com.

[Mike Frysinger]

to wit, since i wanted to update CrOS's pylint, i also updated depot_tools to pylint-3.2 which uses python 3.11.

https://crrev.com/c/6150454

as part of the update, every pylint version has its own rc file.  this gives us a bit more flexibility in which warnings are enabled/disabled by default.  as such, i significantly trimmed the set of disabled warnings in 3.2.  anyone who updates their code to use 3.2 can take care of cleaning things up vs someone (like me) having to globally do it.  that said, the default list is certainly up for discussion, so if something is enabled now that we think is low quality, we can certainly debate adding it back to the default disable list.

-mike