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

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Telemetry
P3
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: Dexter, Assigned: trx, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla55
good-first-bug
Points:
1
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 months ago
+++ 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
(Reporter)

Updated

4 months ago
No longer depends on: 1344852
(Reporter)

Updated

4 months ago
Blocks: 1344854
(Reporter)

Updated

4 months ago
No longer blocks: 1344854
(Reporter)

Updated

4 months ago
Keywords: good-first-bug
(Assignee)

Comment 1

3 months ago
Created attachment 8846786 [details] [diff] [review]
Bug1344853change.patch

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

Updated

3 months ago
Assignee: nobody → kpstsp
(Reporter)

Updated

3 months ago
Status: NEW → ASSIGNED
(Reporter)

Updated

3 months ago
Attachment #8846786 - Flags: review?(alessio.placitelli)
(Reporter)

Comment 2

3 months ago
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.
Attachment #8846786 - Flags: review?(alessio.placitelli)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

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

Updated

3 months ago
Attachment #8846786 - Attachment is obsolete: true
(Reporter)

Comment 6

3 months ago
(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
(Reporter)

Updated

3 months ago
Attachment #8848236 - Flags: review?(alessio.placitelli)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8848236 - Attachment is obsolete: true
(Assignee)

Comment 8

3 months ago
i dropped old commits in histedit and push review  again.
hope i did all correctly)
(Reporter)

Comment 9

3 months ago
mozreview-review
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)?
Attachment #8848235 - Flags: review?(alessio.placitelli)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

3 months ago
hope this patch more readable.

linting test clear. Thanks for you guide!
(Reporter)

Updated

3 months ago
Attachment #8848235 - Attachment is obsolete: true
(Reporter)

Comment 12

3 months ago
mozreview-review
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."
Attachment #8849255 - Flags: review?(alessio.placitelli)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8849255 - Attachment is obsolete: true
(Reporter)

Comment 14

3 months ago
mozreview-review
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!
Attachment #8848235 - Flags: review?(alessio.placitelli) → review+

Comment 15

3 months ago
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ee19e49280f3
Enable flake8 rule E127: "continuation line over-indented for visual indent". r=Dexter

Comment 16

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee19e49280f3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox54: affected → ---
You need to log in before you can comment on or make changes to this bug.