1155969 - xpcom/typelib/ code should follow the flake8 convention

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

Run on the Firefox source code, the following line shows a bunch of warnings. Nothing critical but it would be nice to remove them:
$ flake8 --max-line-length=1000 xpcom/typelib/xpt/tools/


That should be pretty easy to fix for a beginner in Python.
One patch per kind of bug.

Comment 1

[Alex]

Attachment: Xpt/tools/xpt.py Complete Flake8 Compliancy

Hello Sylvestre, 

I would like to address this bug. First I wanted to see if I am on the right track and go through the motions of uploading a patch. I attached the Flake8 compliant xpt.py file for your review. If this looks good, I will break out the changes per Flake8 error type for this file and also apply fixes to runtests.py.

Thank you.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8599685 [details] [diff] [review]
Xpt/tools/xpt.py Complete Flake8 Compliancy

A few things:
* use "?" when asking a review
* I am not the owner of this code, therefor, I cannot review it myself
* You have to provide diff, the whole file is not interesting
* Already, please provide "a diff per kind of changes"
thanks

Comment 3

[Emanuel Hoogeveen [:ehoogeveen]]

Have a look at [1] so you know what to do. [2] and [3] might also be of interest, if you're up for more reading.

[1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
[3] http://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/index.html

Comment 4

[Alex]

Attachment: 1155969_E401.patch

Thank you for the links, Emmanuel. Submitting a patch file for one of the flake8 errors E401. Let me know if this is the correct file type/convention and I will upload the others.

Sylvestre, I'm unsure about who to assign to for review? How do I look up the code's owner?

Comment 5

[Sylvestre Ledru [:Sylvestre]]

(In reply to Alex from comment #4)
> Thank you for the links, Emmanuel. Submitting a patch file for one of the
> flake8 errors E401. Let me know if this is the correct file type/convention
> and I will upload the others.
.hgtags should not be part of the diff.


> Sylvestre, I'm unsure about who to assign to for review? How do I look up
> the code's owner?
Usually, I am looking at the history of a file.

Comment 6

[Alex]

Attachment: 1155969_E401.patch

Assigned for review to file owner. Removed modifications to .hgtags. Fixed flake8 E401 error: multiple imports on a single row

Comment 7

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

Comment on attachment 8601891 [details] [diff] [review]
1155969_E401.patch

I'm not a reviewer for this file, but ted's recently (well, a year ago) reviewed changes to this file. Passing this on to him.

Comment 8

[Alex]

Thanks for the review Ted!

I do have the other patches for all the other pep-8 errors, one per error. However, there are some conflicts when I try to merge the patches. Should I still submit the patches one per error, or should I partially merge some patches so that they can all be merged together with no conflicts?

Comment 9

[Sylvestre Ledru [:Sylvestre]]

(In reply to Alex from comment #8)
> I do have the other patches for all the other pep-8 errors, one per error.
> However, there are some conflicts when I try to merge the patches. Should I
> still submit the patches one per error, or should I partially merge some
> patches so that they can all be merged together with no conflicts?
Yes, one per type of error.
Just rebase them from time to time to avoid conflicts.

Comment 10

[(not currently active) Ted Mielczarek]

If you're working on a single file I would be okay with reviewing one big cleanup patch, honestly. We've done this before in other code (bug 1127376).

Comment 11

[Alex]

Attachment: 1155969_E302.patch

Comment 12

[Alex]

Attachment: 1155969_E125.patch

Comment 13

[Alex]

Attachment: 1155969_E127.patch

Comment 14

[Alex]

Attachment: 1155969_E128.patch

Comment 15

[Alex]

Attachment: 1155969_E131.patch

Comment 16

[Alex]

Attachment: 1155969_E231.patch

Comment 17

[Alex]

Attachment: 1155969_E261.patch

Comment 18

[Alex]

Attachment: 1155969_E265.patch

Comment 19

[Alex]

Attachment: 1155969_E301.patch

Comment 20

[Alex]

Attachment: 1155969_E302.patch

Comment 21

[Alex]

Attachment: 1155969_E303.patch

Comment 22

[Alex]

Attachment: 1155969_E502.patch

Comment 23

[Alex]

Attachment: 1155969_F401.patch

Comment 24

[Alex]

Attachment: 1155969_F841.patch

Comment 25

[Alex]

Attachment: 1155969_W293.patch

Comment 26

[Alex]

Attachment: 1155969_W391.patch

Comment 27

[Alex]

Attachment: 1155969_W602.patch

Comment 28

[Sylvestre Ledru [:Sylvestre]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #10)
> If you're working on a single file I would be okay with reviewing one big
> cleanup patch, honestly. We've done this before in other code (bug 1127376).
Alex, it would have been nice to follow this advice ;)

Comment 29

[Alex]

(In reply to Sylvestre Ledru [:sylvestre] from comment #28)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #10)
> > If you're working on a single file I would be okay with reviewing one big
> > cleanup patch, honestly. We've done this before in other code (bug 1127376).
> Alex, it would have been nice to follow this advice ;)

Apologies, I didn't know if we still wanted one per flake8 bug or all together, so lets have it all! Putting the patches together, should be quick.

Comment 30

[Alex]

Attachment: Modifications to make runtests.py file flake8 compliant

Comment 31

[Alex]

Attachment: Modifications to make xpt.py file flake8 compliant

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Requesting the checkin-needed to land these patches asap (will have to be rebased otherwise)

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Alex, could you remove the request for review which are no longer relevant? Thanks

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c2597bb3edb
https://hg.mozilla.org/integration/mozilla-inbound/rev/9185ec7daf03

Comment 35

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

https://hg.mozilla.org/mozilla-central/rev/3c2597bb3edb
https://hg.mozilla.org/mozilla-central/rev/9185ec7daf03