1344853 - Enable flake8 rule E127: "continuation line over-indented for visual indent"

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Alessio Placitelli [:Dexter]]

+++ This bug was initially created as a clone of Bug #1344852 +++

In bug 1332651 we landed flake8 initial support to lint Python files in the Telemetry directory, by disabling all the detected problems.

This bug is about enabling the E127, "continuation line over-indented for visual indent":

1) Remove the E127 rule from the .flake8 file in https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry

2) Run "./mach lint -l flake8 toolkit/components/telemetry".

3) Fix the reported problems

Comment 1

[Konstantin (:trx)]

Attachment: Bug1344853change.patch

Hi Alessio! please review the patch for this bug. i still in process configuration my hgrc for send review over MozReview

Comment 2

[Alessio Placitelli [:Dexter]]

Comment on attachment 8846786 [details] [diff] [review]
Bug1344853change.patch

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

Hi Kostantin, thank you for submitting this patch!

Please note that, to request a review, you should set the proper flag when attaching your patch (e.g. choose the "?" character from the drop down box near the review field and write in the name/handle for the reviewer), otherwise the request might be not noticed!

Also, please fix the commit message by changing it to:

Bug 1344853 - Enable flake8 rule E127: "continuation line over-indented for visual indent". r?dexter

When a review is requested and has not been granted yet, we use "r?<reviewer name>" instead of "r=<reviewer name>". The latter indicates that a positive review was granted.

::: toolkit/components/telemetry/.flake8
@@ +1,4 @@
>  [flake8]
>  # See http://pep8.readthedocs.io/en/latest/intro.html#configuration
> +ignore = E121, E123, E126, E129, E133, E226, E241, E242, E704, W503, E402, E703, E502, E128, E501, E713, E231, E111, E261, E222, E203, E201, E202, W602, F841, F401, W601
> +#E127

Let's simply remove the E127 rule, there's no need to leave it around, commented.

::: toolkit/components/telemetry/gen-event-data.py
@@ +75,4 @@
>          print("  // objects: [%s]" % ", ".join(e.objects), file=output)
>  
>          # Write the common info structure
> +        print(

We don't need to put the string on a new line. We can leave it on this line and simple properly align the lines below.

For example:

(string_table.stringIndex(e.category),

should be aligned with the " character on the previous line. The lines below should be properly indentent too.

@@ +97,4 @@
>  
>      for common_info_index,e in enumerate(events):
>          for method_name, object_name in itertools.product(e.methods, e.objects):
> +            print(

Same here.

@@ +104,2 @@
>  
> +            print(

Same here.

Comment 3

[Konstantin (:trx)]

Attachment: Bug 1344853 - Enable flake8 rule E127: "continuation line over-indented for visual indent".

Review commit: https://reviewboard.mozilla.org/r/121174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121174/

Comment 4

[Konstantin (:trx)]

Attachment: Bug 1344853 - Enable flake8 rule E127: "continuation line over-indented for visual indent".

Review commit: https://reviewboard.mozilla.org/r/121176/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121176/

Comment 5

[Konstantin (:trx)]

Hi Alessio! I send my commits (with last correction) throu MozReview.
You comments very useful. Thanks  a lot!

Comment 6

[Alessio Placitelli [:Dexter]]

(In reply to Konstantin (:trx) from comment #5)
> Hi Alessio! I send my commits (with last correction) throu MozReview.
> You comments very useful. Thanks  a lot!

You need to squash the commits, otherwise you're requesting the review for two commits on the same lines of code :)

See https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html#changing-code-after-reviews

Comment 7

[Konstantin (:trx)]

Comment on attachment 8848235 [details]
Bug 1344853 - Enable flake8 rule E127: "continuation line over-indented for visual indent".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121174/diff/1-2/

Comment 8

[Konstantin (:trx)]

i dropped old commits in histedit and push review  again.
hope i did all correctly)

Comment 9

[Alessio Placitelli [:Dexter]]

Comment on attachment 8848235 [details]
Bug 1344853 - Enable flake8 rule E127: "continuation line over-indented for visual indent".

https://reviewboard.mozilla.org/r/121174/#review123732

Thank you for the patch! We're almost there, just a few tweaks left, as noted below.

Also, would you mind rebasing your patch and running the linting command again before pushing for review?

::: toolkit/components/telemetry/gen-event-data.py:80
(Diff revision 2)
> -                 string_table.stringIndex(e.expiry_version),
> +              string_table.stringIndex(e.expiry_version),
> -                 extras[0], # extra keys index
> +              extras[0], # extra keys index
> -                 extras[1], # extra keys count
> +              extras[1], # extra keys count
> -                 e.expiry_day,
> +              e.expiry_day,
> -                 e.dataset,
> +              e.dataset,
> -                 " | ".join(e.record_in_processes_enum)),
> +              " | ".join(e.record_in_processes_enum)),

Can you add a whitespace to the left of these lines? (80-85)

That would still be ok for linting and would make them more readable.

::: toolkit/components/telemetry/gen-event-data.py:105
(Diff revision 2)
> -                     string_table.stringIndex(method_name),
> +                  string_table.stringIndex(method_name),
> -                     string_table.stringIndex(object_name)),
> +                  string_table.stringIndex(object_name)),

Can you add a whitespace to the left of this two lines (105, 106)?

Comment 10

[Konstantin (:trx)]

Attachment: Bug 1344853 - Enable flake8 rule E127: "make more readable. continuation line over-indented for visual indent".

Review commit: https://reviewboard.mozilla.org/r/122076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122076/

Comment 11

[Konstantin (:trx)]

hope this patch more readable.

linting test clear. Thanks for you guide!

Comment 12

[Alessio Placitelli [:Dexter]]

Comment on attachment 8849255 [details]
Bug 1344853 - Enable flake8 rule E127: "make more readable. continuation line over-indented for visual indent".

https://reviewboard.mozilla.org/r/122076/#review124854

It looks like this patch is not removing the E127 rule from the .flake8 file. Did you lost it when pushing the update?
It doesn't apply cleanly to the latest tip too. Would you please rebase it, make sure it applies cleanly and run the flake8 tests again?

As it is, it will fail because it's based on an old version of the files :)

::: commit-message-af133:1
(Diff revision 1)
> +Bug 1344853 - Enable flake8 rule E127: "make more readable. continuation line over-indented for visual indent". r?dexter

nit: remove "make more readable."

Comment 13

[Konstantin (:trx)]

Comment on attachment 8848235 [details]
Bug 1344853 - Enable flake8 rule E127: "continuation line over-indented for visual indent".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121174/diff/2-3/

Comment 14

[Alessio Placitelli [:Dexter]]

Comment on attachment 8848235 [details]
Bug 1344853 - Enable flake8 rule E127: "continuation line over-indented for visual indent".

https://reviewboard.mozilla.org/r/121174/#review125274

This looks great now, thanks!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee19e49280f3
Enable flake8 rule E127: "continuation line over-indented for visual indent". r=Dexter

Comment 16

[Carsten Book [:Tomcat]]

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