1 |
On 7/29/20 3:14 PM, Alec Warner wrote: |
2 |
> Hi, |
3 |
> |
4 |
> Recently I've begun to run pylint on the portage codebase. You can see |
5 |
> some recent PRs on this[0][1][2]. Most of the linter errors I've |
6 |
> fixed are what I consider 'fairly trivial'. In general I'm happy to |
7 |
> disable errors (or instances of errors) in addition to resolving them. |
8 |
> You can see some of this |
9 |
> in https://github.com/gentoo/portage/pull/593/files where we just |
10 |
> blanket disable a bunch of very numerous messages (either because I |
11 |
> don't expect to ever fix them, or because they are more work that I |
12 |
> think we should do at the moment.) |
13 |
> |
14 |
> So I see essentially a few choices: |
15 |
> - (a) Do we fix errors in certain classes and run the linter as part of |
16 |
> CI. This means that once we resolve errors of a certain class, we can |
17 |
> enable that class of check and prevent new occurrences. |
18 |
|
19 |
Yes, this is an extremely useful feature to included in our CI. |
20 |
|
21 |
> - (b) Do we just avoid running the linter as part of CI (perhaps |
22 |
> because we are not far enough along on this journey) and focus our |
23 |
> efforts to get to a point where we can do (a) without spamming CI? |
24 |
|
25 |
I think it will be best if we arrange it so that we're not spammed with |
26 |
recurring messages in every CI run. Otherwise, useful messages will be |
27 |
difficult to recognize since they'll be drowned in noise. |
28 |
|
29 |
> - (c) What messages should we bother not fixing at all so we can just |
30 |
> not work on those classes of error? |
31 |
> |
32 |
> As an example of (c); I believe 'protected-access' is likely intrusive |
33 |
> to fix and pointless for portage (cat is out of the bag) where as a |
34 |
> message like W0612(unused-variable) is plausibly something we should fix |
35 |
> throughout the codebase. |
36 |
|
37 |
Sounds good. |
38 |
|
39 |
> Similarly I think "E0602(undefined-variable)" |
40 |
> is often a bug in how pylint treats our lazy initialized variables, so |
41 |
> most of these are false positives. |
42 |
|
43 |
I think it would be nice if we could enable the undefined-variable check. |
44 |
-- |
45 |
Thanks, |
46 |
Zac |