Enable Python code linting in Telemetry

RESOLVED FIXED in Firefox 55

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Dexter, Assigned: djmdeveloper060796, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

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

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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
(Reporter)

Updated

2 years ago
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
(Reporter)

Comment 3

2 years ago
: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)

Comment 6

2 years ago
I love when someone else steals my needinfo requests. Thank you, ahal!
Mentor: alessio.placitelli
Whiteboard: [measurement:client] [lang=python]
(Assignee)

Comment 7

2 years ago
@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)
(Reporter)

Comment 9

2 years ago
(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)
(Assignee)

Comment 10

2 years ago
@Alessio Thanks a lot for the pointers, I shall upload a patch soon.
(Assignee)

Comment 11

2 years ago
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.
Attachment #8843378 - Flags: review?(alessio.placitelli)
(Reporter)

Comment 12

2 years ago
(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?
(Assignee)

Comment 13

2 years ago
Created attachment 8843904 [details]
error_lint.txt

I have attached a file containing the errors I encountered.
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 14

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

Updated

2 years ago
Assignee: nobody → djmdeveloper060796
Flags: needinfo?(alessio.placitelli)
(Reporter)

Comment 15

2 years ago
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+
(Reporter)

Updated

2 years ago
Blocks: 1344709
(Reporter)

Comment 17

2 years ago
Looks like the F8 job on try was successful, this is ready to land!
Keywords: checkin-needed
(Assignee)

Comment 18

2 years ago
great :)
(Reporter)

Updated

2 years ago
Blocks: 1344831
(Reporter)

Updated

2 years ago
Blocks: 1344832
(Reporter)

Updated

2 years ago
Blocks: 1344833
(Reporter)

Updated

2 years ago
Blocks: 1344834
(Reporter)

Updated

2 years ago
Blocks: 1344836
(Reporter)

Updated

2 years ago
Blocks: 1344840
(Reporter)

Updated

2 years ago
Blocks: 1344841
(Reporter)

Updated

2 years ago
Blocks: 1344842
(Reporter)

Updated

2 years ago
Blocks: 1344844
(Reporter)

Updated

2 years ago
Blocks: 1344846
(Reporter)

Updated

2 years ago
Blocks: 1344849
(Reporter)

Updated

2 years ago
Blocks: 1344850
(Reporter)

Updated

2 years ago
Blocks: 1344852
(Reporter)

Updated

2 years ago
Blocks: 1344853
(Reporter)

Updated

2 years ago
Blocks: 1344854
(Reporter)

Updated

2 years ago
Blocks: 1344855
(Reporter)

Updated

2 years ago
Blocks: 1344858

Comment 19

2 years ago
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

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/48cf9ac6ab31
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox53: affected → ---
You need to log in before you can comment on or make changes to this bug.