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

RESOLVED FIXED in Firefox 55

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years 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])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

2 years ago
No longer depends on: 1344852
(Reporter)

Updated

2 years ago
Blocks: 1344854
(Reporter)

Updated

2 years ago
No longer blocks: 1344854
(Reporter)

Updated

2 years ago
Keywords: good-first-bug
(Assignee)

Comment 1

2 years ago
Posted patch Bug1344853change.patch (obsolete) — Splinter Review
Hi Alessio! please review the patch for this bug. i still in process configuration my hgrc for send review over MozReview
(Reporter)

Updated

2 years ago
Assignee: nobody → kpstsp
(Reporter)

Updated

2 years ago
Status: NEW → ASSIGNED
(Reporter)

Updated

2 years ago
Attachment #8846786 - Flags: review?(alessio.placitelli)
(Reporter)

Comment 2

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

Comment 5

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

Updated

2 years ago
Attachment #8846786 - Attachment is obsolete: true
(Reporter)

Comment 6

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

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

Updated

2 years ago
Attachment #8848236 - Attachment is obsolete: true
(Assignee)

Comment 8

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

Comment 9

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

2 years ago
hope this patch more readable.

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

Updated

2 years ago
Attachment #8848235 - Attachment is obsolete: true
(Reporter)

Comment 12

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

2 years ago
Attachment #8849255 - Attachment is obsolete: true
(Reporter)

Comment 14

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

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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee19e49280f3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years 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.