Open Bug 1199939 Opened 7 years ago Updated 5 months ago

Automatically check for PEP 8 errors in Python code

Categories

(Firefox Build System :: Mach Core, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

People

(Reporter: samy, Unassigned, Mentored)

Details

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.107 Safari/537.36
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
Attached patch mach_flake8.patch (obsolete) — Splinter Review
Mentor: sledru
Component: Untriaged → mach
Product: Firefox → Core
Version: unspecified → Trunk
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?
> * 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.
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.
Assignee: nobody → samy
Added a search_files() function for multi-platform search.
Attachment #8654527 - Attachment is obsolete: true
Attachment #8679194 - Attachment is obsolete: true
Flags: needinfo?(sledru)
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!
(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
Flags: needinfo?(sledru)
Product: Core → Firefox Build System
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

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

Assignee: samy → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.