Closed Bug 1344853 Opened 4 years ago Closed 3 years ago

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

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement
Points:
1

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: Dexter, Assigned: trx, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [measurement:client][lang=python])

Attachments

(1 file, 3 obsolete files)

+++ 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
No longer depends on: 1344852
Blocks: 1344854
No longer blocks: 1344854
Keywords: good-first-bug
Attached 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
Assignee: nobody → kpstsp
Status: NEW → ASSIGNED
Attachment #8846786 - Flags: review?(alessio.placitelli)
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)
Hi Alessio! I send my commits (with last correction) throu MozReview.
You comments very useful. Thanks  a lot!
Attachment #8846786 - Attachment is obsolete: true
(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
Attachment #8848236 - Flags: review?(alessio.placitelli)
Attachment #8848236 - Attachment is obsolete: true
i dropped old commits in histedit and push review  again.
hope i did all correctly)
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)
hope this patch more readable.

linting test clear. Thanks for you guide!
Attachment #8848235 - Attachment is obsolete: true
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)
Attachment #8849255 - Attachment is obsolete: true
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+
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
https://hg.mozilla.org/mozilla-central/rev/ee19e49280f3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.