1356527 - Verify the remaining exceptions in .flake8

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Alessio Placitelli [:Dexter]]

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

Comment 1

[Alessio Placitelli [:Dexter]]

Avikalpa, would you be interested in working on this bug?

Comment 2

[Avikalpa Kundu [:kalpa]]

Yes, I'll be working on it.
Thanks.

Comment 3

[Alessio Placitelli [:Dexter]]

Avikalpa, are you still interested in working on this bug?

Comment 4

[Avikalpa Kundu [:kalpa]]

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. :)

Comment 5

[Avikalpa Kundu [:kalpa]]

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?

Comment 6

[Alessio Placitelli [:Dexter]]

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

Comment 7

[Avikalpa Kundu [:kalpa]]

Attachment: 1356527.patch

New bug please :)

Comment 8

[Alessio Placitelli [:Dexter]]

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"

Comment 9

[Avikalpa Kundu [:kalpa]]

Attachment: 1356527.patch

Comment 10

[Alessio Placitelli [:Dexter]]

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

Comment 11

[Carsten Book [:Tomcat]]

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)

Comment 12

[Avikalpa Kundu [:kalpa]]

(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 <[e-mail]>
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?

Comment 13

[Carsten Book [:Tomcat]]

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.

Comment 14

[Avikalpa Kundu [:kalpa]]

Yes. That seems to be the case.

Comment 15

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf749aa49b8344a31d3ad6f7407d64b3e0d5f698
Bug 1356527 - Enable the remaining rules in Telemetry .flake8; r=Dexter

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf749aa49b83
Enable the remaining rules in Telemetry .flake8; r=Dexter

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bf749aa49b83