1332651 - Enable Python code linting in Telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Alessio Placitelli [:Dexter]]

We do have a few Python files to do scalars/histograms parsing in the client [1]. It would be great if we could settle on a minimum set of linting rules and make all the Python files we own pass them.

The rules could be shared between the client and the pipeline, as some files are shared among the services (e.g. parse_scalars.py).

Mauro mentioned using flak8 to do python linting on ATMO, and provided the set of rules they are using right now [2].

Frank suggested using a global pylint file for consistency between the client and the pipeline.

[1] - http://searchfox.org/mozilla-central/search?q=&case=false&regexp=false&path=telemetry%2F*.py
[2] - https://github.com/mozilla/telemetry-analysis-service/blob/master/setup.cfg

Comment 1

[Frank Bertsch [:frank]]

Any requirements for linting we should add in the contributing doc [0]. Additionally, we should probably include our global pylint/flake8 file there, which every repo can then use for linting.

Re: pylint vs. flake8, I'm okay with either. In their base configurations, pylint seems to report much more, which can be a good or bad thing, depending on how pedantic we want to be.

[0] https://github.com/mozilla/telemetry-onboarding/blob/master/Contributing.md

Comment 2

[Frank Bertsch [:frank]]

It would also be nice if we could integrate this with the tools we already use to check our code; mainly Travis. An example for flake8 is at [0].

[0] https://github.com/frewsxcv/python-geojson/pull/46/commits/e6d3b1e0393ebf9031dea6c44b75f845aa784b4b

Comment 3

[Alessio Placitelli [:Dexter]]

:gps, you know quite a bit of Python and our build system. Do you think it could be worthwhile integrating Python code linting through flake8 (or pylint) in our build system, as done with "./mach eslint"?

Any comment/feedback/idea?

Comment 4

[Jonathan Griffin (:jgriffin)]

cc'ing ahal, as he's done a lot of work with eslint

Comment 5

[Andrew Halberstadt [:ahal]]

This already exists :)

Just add any directories you want here:
https://dxr.mozilla.org/mozilla-central/source/tools/lint/flake8.lint#178

The default configuration lives in topsrcdir/.flake8, but you can create a new .flake8 file to change it (note: doing this will replace the default config, not update it).

After adding the path, when you push to hg your results will appear in the 'f8' task in treeherder. To run locally, use:
./mach lint -l flake8 <path/to/dir>

(Note: the -l flake8 is optional depending if you want to run all linters configured for that directory or just flake8)

In fact, ./mach eslint is just an alias to ./mach lint -l eslint

Comment 6

[Gregory Szorc [:gps]]

I love when someone else steals my needinfo requests. Thank you, ahal!

Comment 7

[Deepjyoti Mondal]

@Georg, Hi, I am new to Telemetry, I would like to take up this bug. How should I proceed, I have a firefox artifact build running.

Comment 8

[Georg Fritzsche [:gfritzsche]]

I'll redirect this to Alessio, who is the mentor for this bug.

Comment 9

[Alessio Placitelli [:Dexter]]

(In reply to Deepjyoti Mondal from comment #7)
> @Georg, Hi, I am new to Telemetry, I would like to take up this bug. How
> should I proceed, I have a firefox artifact build running.

Hi Deepijoty Mondal,

for enabling Python linting we need to:

1) Add the Telemetry directory, 'toolkit/components/telemetry', at the end of https://dxr.mozilla.org/mozilla-central/source/tools/lint/flake8.lint#178
2) Run "./mach lint -l flake8 toolkit/components/telemetry" to see if our Python scripts pass the linting process with the default rules defined at https://dxr.mozilla.org/mozilla-central/source/.flake8
3) If the previous step produces no error, then we're ok and can simply land this (I don't want to spoil the fun but this is unlikely to happen :) ). If it fails, then we should create a new .flake8 file (with a structure similar to https://dxr.mozilla.org/mozilla-central/source/.flake8 ) in 'toolkit/components/telemetry' that disables the offending check/rule. We will have to file a bugs to enable these checks as follow-ups.

Please do not hesitate to ask if you need more information or get stuck!

Comment 10

[Deepjyoti Mondal]

@Alessio Thanks a lot for the pointers, I shall upload a patch soon.

Comment 11

[Deepjyoti Mondal]

Attachment: bug1332651.patch

I added toolkit/components/telemetry in flake8.lint file but it produced lot errors on running ./mach lint -l flake8 toolkit/components/telemetry so I created a new .flake8 file at toolkit/components/telemetry at ignored the trouble causing rules. Hope this helps.

Comment 12

[Alessio Placitelli [:Dexter]]

(In reply to Deepjyoti Mondal from comment #11)
> Created attachment 8843378 [details] [diff] [review]
> bug1332651.patch
> 
> I added toolkit/components/telemetry in flake8.lint file but it produced lot
> errors on running ./mach lint -l flake8 toolkit/components/telemetry so I
> created a new .flake8 file at toolkit/components/telemetry at ignored the
> trouble causing rules. Hope this helps.

Thanks Deepijoty! Would you mind attaching a text file with the errors it produced?

Comment 13

[Deepjyoti Mondal]

Attachment: error_lint.txt

I have attached a file containing the errors I encountered.

Comment 14

[Deepjyoti Mondal]

@Alessio Do let me know when you create the follow up issues to solve the linting errors. I would love to work on them :)

Comment 15

[Alessio Placitelli [:Dexter]]

Comment on attachment 8843378 [details] [diff] [review]
bug1332651.patch

Review of attachment 8843378 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thanks!

Comment 16

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd541e18a092

Comment 17

[Alessio Placitelli [:Dexter]]

Looks like the F8 job on try was successful, this is ready to land!

Comment 18

[Deepjyoti Mondal]

great :)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48cf9ac6ab31
Enabled python code linting in Telemetry. r=Dexter

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48cf9ac6ab31