Closed Bug 1332651 Opened 5 years ago Closed 5 years ago

Enable Python code linting in Telemetry

Categories

(Toolkit :: Telemetry, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: Dexter, Assigned: djmdeveloper060796, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client] [lang=python])

Attachments

(2 files)

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
Blocks: 1201022
Priority: -- → P3
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
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
: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?
Flags: needinfo?(gps)
cc'ing ahal, as he's done a lot of work with eslint
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
Flags: needinfo?(gps)
I love when someone else steals my needinfo requests. Thank you, ahal!
Mentor: alessio.placitelli
Whiteboard: [measurement:client] [lang=python]
@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.
Flags: needinfo?(gfritzsche)
I'll redirect this to Alessio, who is the mentor for this bug.
Flags: needinfo?(gfritzsche) → needinfo?(alessio.placitelli)
(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!
Flags: needinfo?(alessio.placitelli)
@Alessio Thanks a lot for the pointers, I shall upload a patch soon.
Attached patch bug1332651.patchSplinter Review
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.
Attachment #8843378 - Flags: review?(alessio.placitelli)
(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?
Attached file error_lint.txt
I have attached a file containing the errors I encountered.
Flags: needinfo?(alessio.placitelli)
@Alessio Do let me know when you create the follow up issues to solve the linting errors. I would love to work on them :)
Assignee: nobody → djmdeveloper060796
Flags: needinfo?(alessio.placitelli)
Comment on attachment 8843378 [details] [diff] [review]
bug1332651.patch

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

This looks good, thanks!
Attachment #8843378 - Flags: review?(alessio.placitelli) → review+
Blocks: 1344709
Looks like the F8 job on try was successful, this is ready to land!
Keywords: checkin-needed
great :)
Blocks: 1344831
Blocks: 1344832
Blocks: 1344833
Blocks: 1344834
Blocks: 1344836
Blocks: 1344840
Blocks: 1344841
Blocks: 1344842
Blocks: 1344844
Blocks: 1344846
Blocks: 1344849
Blocks: 1344850
Blocks: 1344852
Blocks: 1344853
Blocks: 1344854
Blocks: 1344855
Blocks: 1344858
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48cf9ac6ab31
Enabled python code linting in Telemetry. r=Dexter
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/48cf9ac6ab31
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.