Closed
Bug 1356527
Opened 8 years ago
Closed 7 years ago
Verify the remaining exceptions in .flake8
Categories
(Toolkit :: Telemetry, enhancement, P3)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: Dexter, Assigned: kalpa)
References
Details
(Whiteboard: [measurement:client])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
After all the work carried over in the dependencies of bug 1344709, most of the linting problems with the Python code were solved.
However, the flake8 file in toolkit/components/telemetry still has a few active rules [1]:
> ignore = E121, E123, E126, E129, E133, E226, E241, E242, E704, W503, E402, E501, W601
Except for rule E501 (which is being taken care of in bug 1344834), we should check if these rules are still relevant. If removing the rule still reports an error and the error it's trivial to fix, we should fix it here and remove the rule.
To run flake8 linting, the following command can be used:
> ./mach lint -l flake8 toolkit/components/telemetry
[1] - http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/.flake8
Reporter | ||
Comment 1•8 years ago
|
||
Avikalpa, would you be interested in working on this bug?
Blocks: 1344709
Points: --- → 1
Flags: needinfo?(avikalpakundu)
Priority: -- → P3
Whiteboard: [measurement:client]
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → avikalpakundu
Reporter | ||
Comment 3•7 years ago
|
||
Avikalpa, are you still interested in working on this bug?
Flags: needinfo?(avikalpakundu)
Assignee | ||
Comment 4•7 years ago
|
||
Yes. I'll do it.
Apologies for 12 days of delay although I claimed to be free this month.
To be honest, I haven't looked at it yet but I'll send a patch today. :)
Flags: needinfo?(avikalpakundu)
Assignee | ||
Comment 5•7 years ago
|
||
Error Report
------------
W601
/mozilla/mozilla-unified/toolkit/components/telemetry/histogram_tools.py
504:30 error .has_key() is deprecated, use 'in' W601 (flake8)
E402
/mozilla/mozilla-unified/toolkit/components/telemetry/histogram_tools.py
34:1 error module level import not at top of file E402 (flake8)
W503
-- nil
E704
-- nil
E242
-- nil
E241
-- nil
E226
-- nil
E133
-- nil
E129
-- nil
E126
/mozilla/mozilla-unified/toolkit/components/telemetry/gen-event-data.py
49:21 error continuation line over-indented for hanging indent E126 (flake8)
E123
/mozilla/mozilla-unified/toolkit/components/telemetry/gen-histogram-bucket-ranges
31:13 error closing bracket does not match indentation of opening bracket's line E123 (flake8)
/mozilla/mozilla-unified/toolkit/components/telemetry/gen-histogram-data.py
128:9 error closing bracket does not match indentation of opening bracket's line E123 (flake8)
E121
-- nil
------ end of report
E123 & E126 are trivial and I'll send a patch on them here.
I guess E402 is also trivial to fix.
Am I correct on my understanding?
Flags: needinfo?(alessio.placitelli)
Reporter | ||
Comment 6•7 years ago
|
||
It looks (In reply to Avikalpa Kundu [:kalpa] from comment #5)
> Error Report
> ------------
> W601
> /mozilla/mozilla-unified/toolkit/components/telemetry/histogram_tools.py
> 504:30 error .has_key() is deprecated, use 'in' W601 (flake8)
It looks like you're using an old revision of mozilla-central, because this was fixed in bug 1344858.
> E402
> /mozilla/mozilla-unified/toolkit/components/telemetry/histogram_tools.py
> 34:1 error module level import not at top of file E402 (flake8)
> E126
> /mozilla/mozilla-unified/toolkit/components/telemetry/gen-event-data.py
> 49:21 error continuation line over-indented for hanging indent E126
> (flake8)
>
> E123
> /mozilla/mozilla-unified/toolkit/components/telemetry/gen-histogram-bucket-
> ranges
> 31:13 error closing bracket does not match indentation of opening
> bracket's line E123 (flake8)
>
> /mozilla/mozilla-unified/toolkit/components/telemetry/gen-histogram-data.py
> 128:9 error closing bracket does not match indentation of opening
> bracket's line E123 (flake8)
Yes, you got it right. All of these seem to be reasonably trivial to fix. Please make sure they are still happening on the latest revision of mozilla-central, then provide a patch to fix them and enable the relative rule
Flags: needinfo?(alessio.placitelli)
Assignee | ||
Comment 7•7 years ago
|
||
New bug please :)
Attachment #8862437 -
Flags: review?(alessio.placitelli)
Reporter | ||
Comment 8•7 years ago
|
||
Comment on attachment 8862437 [details] [diff] [review]
1356527.patch
Review of attachment 8862437 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good, but the commit message needs to be changed, as it should explain what the commit does rather than the scope of the bug :) Moreover, the end of the commit message should be "r?dexter", with a question mark rather than a "=". The latter indicates that the code was reviewed and not r+'d yet.
Change it to something like: "Bug 1356527 - Enable the remaining rules in Telemetry .flake8. r?Dexter"
Attachment #8862437 -
Flags: review?(alessio.placitelli) → feedback+
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8862437 -
Attachment is obsolete: true
Attachment #8862526 -
Flags: review?(alessio.placitelli)
Reporter | ||
Updated•7 years ago
|
Attachment #8862526 -
Flags: review?(alessio.placitelli) → review+
Reporter | ||
Comment 10•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 11•7 years ago
|
||
has problems to apply:
atching file toolkit/components/telemetry/gen-histogram-data.py
Hunk #1 FAILED at 119
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/telemetry/gen-histogram-data.py.rej
patching file toolkit/components/telemetry/histogram_tools.py
Hunk #1 FAILED at 4
Hunk #2 FAILED at 25
2 out of 2 hunks FAILED -- saving rejects to file toolkit/components/telemetry/histogram_tools.py.rej
patch failed, unable to continue (try -v)
Flags: needinfo?(avikalpakundu)
Keywords: checkin-needed
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #11)
> has problems to apply:
>
> atching file toolkit/components/telemetry/gen-histogram-data.py
> Hunk #1 FAILED at 119
> 1 out of 1 hunks FAILED -- saving rejects to file
> toolkit/components/telemetry/gen-histogram-data.py.rej
> patching file toolkit/components/telemetry/histogram_tools.py
> Hunk #1 FAILED at 4
> Hunk #2 FAILED at 25
> 2 out of 2 hunks FAILED -- saving rejects to file
> toolkit/components/telemetry/histogram_tools.py.rej
> patch failed, unable to continue (try -v)
It looks like you're using an old revision of mozilla-central.
My tree has :
changeset: 395371:7b4d43ed9b7f
fxtree: inbound
user: Geoff Brown <gbrown@mozilla.com>
date: Thu Apr 27 07:52:58 2017 -0600
summary: Bug 1359541 - Enable eslint on mobile/android/tests; r=standard8,snorp
As last commit.
Am I mistaken?
Flags: needinfo?(avikalpakundu) → needinfo?(cbook)
Comment 13•7 years ago
|
||
i guess the problem is that https://bugzilla.mozilla.org/show_bug.cgi?id=1357001 landed before your patch and is now causing the conflicts.
Flags: needinfo?(cbook) → needinfo?(avikalpakundu)
Reporter | ||
Comment 15•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf749aa49b8344a31d3ad6f7407d64b3e0d5f698
Bug 1356527 - Enable the remaining rules in Telemetry .flake8; r=Dexter
Comment 16•7 years ago
|
||
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf749aa49b83
Enable the remaining rules in Telemetry .flake8; r=Dexter
Comment 17•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•