Enabling More Pylint Checks In Chromium

[Brian Sheedy]

Hi all,

The majority of Chromium Python code currently uses the default pylintrc files provided by depot_tools. However, the file used by Pylint 2.7 calls out the fact that there are many checks which should be enabled but are currently disabled because there are too many violations in existing code. The list of disabled checks for Pylint 3.2 is significantly shorter, but still has quite a few useful checks disabled.

For comparison, the GPU test code uses its own pylintrc which has the number of disabled checks down to 8.

As originally pitched in crbug.com/355016915, it would be nice if we could reduce the depot_tools file's disable list down to only those that don't make sense for Chromium. However, before we can go ahead with that, we need a couple of things from Python users in Chromium:

1. Agreement that this is something we want to actually do. I'm a big fan of catching as much as possible in the linter, but perhaps others have opinions about how annoying/unhelpful these checks are.
2. Agreement on what the default set of checks should be. I think the list used by the GPU code is probably the lower bound for how small we can make the disable list, but I wouldn't be surprised if a number of checks that are enabled there don't make sense for Chromium in general.

Thoughts?

Thanks,

Brian

[Mike Frysinger]

the biggest reason checks are disabled isn't because we dislike them, but because they flagged existing code.  the people updating pylint didn't want to be on the hook for cleaning the whole codebase (especially code that isn't well tested or executed), and owners of the python code didn't want to spend time on cleaning up new lint warnings.

so unless you have a plan for this, i'm not sure how useful a higher level discussion on which checks to enable.

-mike

--
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/ad0606cf-4a4e-47ac-820c-bcca82b8da20n%40chromium.org.

[Brian Sheedy]

> so unless you have a plan for this, i'm not sure how useful a higher level discussion on which checks to enable.

Update any existing uses of Pylint in PRESUBMIT.py files to point the `pylintrc` argument to a copy of depot_tools' current file in the same directory as the presubmit file. After that's done, no existing code will be using the default pylintrc file, so we can update it to use the new list of checks.

Any code that's added to a new directory after that point will be using the new checks. Existing code can continue to use their pylintrc copy indefinitely, but if the owners ever do want to clean things up, they can use their copy as a burndown list of which checks still need to be enabled.

[Andrew Grieve]

I recently switched a package to the new version in order for newer APIs to be recognized.

https://source.chromium.org/chromium/chromium/src/+/main:build/android/pylintrc-3.2

Looking at the default one you linked, I don't think there are any disabled checks in that list that I would want enabled.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/python/CAAYTr1WvPZp7ybAe__Hpm80zOzR1NB6p1dH%2Bzvr-3EiDVh%3DLPw%40mail.gmail.com.

[Mike Frysinger]

arguably we already have this.  pylint is versioned, and it uses a versioned pylintrc.  there is no `pylint` script.

the canned presubmit checks have a version= setting, and it defaults to 2.7.  a quick cs/ check suggests no one is using anything newer.  no one wants to spend time updating linters and cleaning their code with all the new warnings it triggers.

when i added pylint-3.2, i reset the list of disabled checks so it's much smaller.  so if you want code to run with more linter checks, update the presubmit files to specify version=3.2 instead of trying to keep the ancient 2.7 version limping along.

-mike

[Brian Sheedy]

> Looking at the default one you linked, I don't think there are any disabled checks in that list that I would want enabled.

I see a number that I think would be ideally enabled:

• consider-iterating-dictionary
• consider-using-f-string
• keyword-arg-before-vararg
• no-else-break
• no-else-continue
• no-else-raise
• no-else-return
• no-name-in-module
• reimported
• super-with-arguments
• try-except-raise

Do you have particular reasons for wanting to keep those disabled?

> a quick cs/ check suggests no one is using anything newer.

There are definitely uses of Pylint 3.2 in Chromium, but yes, it seems like most PRESUBMIT.py files are still using 2.7.

Relying on different versioned files seems like a reasonable approach to address this. If/when a newer version of Pylint is added to depot_tools, we can further decrease the disabled checks list.

[Mike Frysinger]

if it's not in presubmit, then it should be free to break

i don't find value in no-else-{break,continue,raise,return} warnings, and have code that i find more readable using the `else` statement

consider-using-f-string is nice for new code, but i have a hard time saying there's value in rewriting existing codebaes

-mike