[Buildroot] [PATCH 1/1] .flake8: fix check for 80/132 columns

[Arnout Vandecappelle]

On 10/04/2019 21:50, Yann E. MORIN wrote:
> Arnout, All,
> 
> On 2019-04-10 12:33 +0200, Arnout Vandecappelle spake thusly:
>> On 09/04/2019 02:17, Ricardo Martincoski wrote:
>>> We recommend wrapping at 80 columns but we accept 132 columns when it
>>> makes more readable.
>>>
>>> When running flake8 locally, use maximum line length 80.
>>> But when running in GitLab CI, keep the check-flake8 job failing only
>>> for lines longer than 132.
>>>
>>> Reported-by: Arnout Vandecappelle <arnout at mind.be>
>>> Signed-off-by: Ricardo Martincoski <ricardo.martincoski at gmail.com>
>>> Cc: Arnout Vandecappelle <arnout at mind.be>
>>> Cc: Peter Korsgaard <peter at korsgaard.com>
>>> Cc: Thomas Petazzoni <thomas.petazzoni at bootlin.com>
>>> Cc: Yann E. MORIN <yann.morin.1998 at free.fr>
>>
>>  Applied to master, thanks.
> 
> You seemed to have missed my comment where I said that was not so good
> an idea;

 Indeed I missed it (I didn't notice my IMAP wasn't updating).

> if you noticed, you could have at least dismissed it here.
> 
> So, why don't I think it is a good idea to differentiate the pipeline
> from the local checks?

 As written in the commit message, but maybe not enough: we really want a limit
of 80 characters. However, sometimes that limit is a little low, so we want to
be lenient about it. Therefore, we can't put this limit in CI, since we would
get errors that we want to ignore. Of course, we could put #noqa comments behind
it, but that would make those long lines even longer and definitely doesn't help
readability.

 So, this is a stopgap that makes sure a developer can still get warnings about
long lines (otherwise you'd have to check it manually), but they can safely
ignore those warnings - up to a limit.


> If contributors pass the checks locally, they'll notice the error
> report, and fix the code so that there is no error. Thus, there is no
> reason to make the pipeline more lenient about the checks.
> 
> If the contributor is allowed to ignore an error reported by the check,

 The point is: we want to treat the a line between 80 and 132 characters as a
warning. Unfortunately, flake8 doesn't allow for such a distinction, so this is
the next best thing.

> on the assumption that the pipleine is more lenient about it, it means
> they will just always ignore errors reported locally (if they even run
> the check to start with) and wait for the pipeline to spit actual
> errors before fixing them, if at all, in which case another contoibutor
> who does run the checks will be polluted by errors others should have
> noticed.

 I don't understand what you mean here...


> So, I still think this was a bad idea to differentiate the two.

 I agree it's not ideal. But what we have now is not ideal either: lines of 85
characters long that could easily have been split.

 Note that this was discussed on the list a long time ago, but I can't find it
back...

 Regards,
 Arnout

> 
> Regards,
> Yann E. MORIN.
> 
>>  My flake8 finds a lot of errors that the one it gitlab-ci doesn't find:
>>
>> 3     E112 expected an indented block
>> 2     E113 unexpected indentation
>> 2     E116 unexpected indentation (comment)
>> 25    E122 continuation line missing indentation or outdented
>> 1     E201 whitespace after '['
>> 2     E202 whitespace before ']'
>> 1     E203 whitespace before ':'
>> 6     E225 missing whitespace around operator
>> 2     E227 missing whitespace around bitwise or shift operator
>> 1     E261 at least two spaces before inline comment
>> 1     E301 expected 1 blank line, found 0
>> 49    E302 expected 2 blank lines, found 1
>> 4     E305 expected 2 blank lines after class or function definition, found 1
>> 2     E711 comparison to None should be 'if cond is None:'
>> 5     E741 ambiguous variable name 'l'
>> 1     E902 IndentationError: unindent does not match any outer indentation level
>> 3     E999 IndentationError: expected an indented block
>> 3     F401 'sys' imported but unused
>> 3     W391 blank line at end of file
>> 126   W605 invalid escape sequence '\+'
>>
>>  But that's of course an entirely different concern.
>>
>>  Regards,
>>  Arnout
>>
>>> ---
>>> The result of check-flake8 does not change:
>>> https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/192997289
>>> ---
>>>  .flake8           | 2 +-
>>>  .gitlab-ci.yml    | 2 +-
>>>  .gitlab-ci.yml.in | 2 +-
>>>  3 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/.flake8 b/.flake8
>>> index 7dd7b541cc..ee3d5035a0 100644
>>> --- a/.flake8
>>> +++ b/.flake8
>>> @@ -2,4 +2,4 @@
>>>  exclude=
>>>      # copied from the kernel sources
>>>      utils/diffconfig
>>> -max-line-length=132
>>> +max-line-length=80
>>> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
>>> index 572868a557..e62e1c3e38 100644
>>> --- a/.gitlab-ci.yml
>>> +++ b/.gitlab-ci.yml
>>> @@ -38,7 +38,7 @@ check-flake8:
>>>          - find * -type f -print0 | xargs -0 file | grep 'Python script' | cut -d':' -f1 >> files.txt
>>>          - sort -u files.txt | tee files.processed
>>>      script:
>>> -        - python -m flake8 --statistics --count $(cat files.processed)
>>> +        - python -m flake8 --statistics --count --max-line-length=132 $(cat files.processed)
>>>      after_script:
>>>          - wc -l files.processed
>>>  
>>> diff --git a/.gitlab-ci.yml.in b/.gitlab-ci.yml.in
>>> index a506840892..b1ec671867 100644
>>> --- a/.gitlab-ci.yml.in
>>> +++ b/.gitlab-ci.yml.in
>>> @@ -38,7 +38,7 @@ check-flake8:
>>>          - find * -type f -print0 | xargs -0 file | grep 'Python script' | cut -d':' -f1 >> files.txt
>>>          - sort -u files.txt | tee files.processed
>>>      script:
>>> -        - python -m flake8 --statistics --count $(cat files.processed)
>>> +        - python -m flake8 --statistics --count --max-line-length=132 $(cat files.processed)
>>>      after_script:
>>>          - wc -l files.processed
>>>  
>>>
>