1199939 - Automatically check for PEP 8 errors in Python code

Status: NEW

(Firefox Build System :: Mach Core, enhancement)

Description

[Samy Dindane]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.107 Safari/537.36

Comment 1

[Samy Dindane]

Hi.

I've had #1142609 as my first bug, where I manually fixed some PEP 8 errors.
Now I think it would be a good idea to have them run automatically, by users as well as the try server, in order to improve the readability of the code and make it consistent across the wide spectrum of the project [1].

I started by writing a mach command (flake8) that runs the tests on a specific directory or file, and wanted to get your opinion and advice about this potential feature.

Thank you.

[1]: https://www.python.org/dev/peps/pep-0008/#a-foolish-consistency-is-the-hobgoblin-of-little-minds

Comment 2

[Samy Dindane]

Attachment: mach_flake8.patch

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8654527 [details] [diff] [review]
mach_flake8.patch

>     'jetpack': {
>-        'aliases': ('J',),
>+        'aliases': ('f',),
>         'mach_command': 'jetpack-test',
>         'kwargs': {},
I don't think you want to do this.
me + '.exe' if sys.platform.startswith('win') else name
> 
>+
>+@CommandProvider
>+class Flake8Command(MachCommandBase):
>+    @Command('flake8', category='testing', description='Run flake8 in order to find PEP 8 errors in Python code.')
-in order
>+        command = "flake8 --max-line-length=220 --select=%s $(find %s -iname '*.py'|grep -v tests)" % (filters, target)
Several things here:
* Why 220?
* What if I want to give different options?
* What is the file I want to check is called run_tests.py?
* What if I want to run on it on a test files/directory?

Comment 4

[Samy Dindane]

> * Why 220?

I'm gonna remove that line if that's okay with you.

> * What if I want to give different options?

How should it look like in your opinion?
What about `./mach flake8 testing/mach_commands.py e501 --args="--max-line-length=220"`?

Ok for the rest.

Comment 5

[Samy Dindane]

Attachment: First patch's changes + Sylv's suggestions + --arg param

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Sorry Samy, I didn't see your activity on this bug. Next time, please needinfo-me :)

* I do think that we should have max-line-length by default. I just meant that we should disable this check. Not 220.
* find %s -iname '*.py => I don't think we should do that. Will it work on Windows?
I think we should instead do that using Python code.

Hope this helps.

Comment 7

[Samy Dindane]

Attachment: mach_flake8.patch

Added a search_files() function for multi-platform search.

Comment 8

[Samy Dindane]

Hi Sylvestre,

> I do think that we should have max-line-length by default

Ok. What value do you suggest?

> I just meant that we should disable this check. Not 220.

I don't understand. What check exactly?

Thanks!

Comment 9

[Sylvestre Ledru [:Sylvestre]]

(In reply to Samy Dindane from comment #8)
> Hi Sylvestre,
> 
> > I do think that we should have max-line-length by default
> 
> Ok. What value do you suggest?
If it was possible, we could disable the check but I don't know if it is possible.
Otherwise, just a big number like 300 (random number)

Besides that, I missed the info but instead of using subprocess.call
we should use the api
http://flake8.readthedocs.org/en/latest/api.html

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.