Closed Bug 1155969 Opened 5 years ago Closed 4 years ago

xpcom/typelib/ code should follow the flake8 convention

Categories

(Core :: XPCOM, defect, trivial)

x86_64
Linux
defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: Sylvestre, Assigned: alexlee0227, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=Python])

Attachments

(20 files, 2 obsolete files)

848 bytes, patch
ted
: review+
Details | Diff | Splinter Review
12.31 KB, patch
Details | Diff | Splinter Review
1.39 KB, patch
Details | Diff | Splinter Review
1.30 KB, patch
Details | Diff | Splinter Review
2.26 KB, patch
Details | Diff | Splinter Review
1.38 KB, patch
Details | Diff | Splinter Review
843 bytes, patch
Details | Diff | Splinter Review
2.03 KB, patch
Details | Diff | Splinter Review
7.43 KB, patch
Details | Diff | Splinter Review
1.48 KB, patch
Details | Diff | Splinter Review
12.31 KB, patch
Details | Diff | Splinter Review
1.10 KB, patch
Details | Diff | Splinter Review
1.19 KB, patch
Details | Diff | Splinter Review
1.31 KB, patch
Details | Diff | Splinter Review
1.11 KB, patch
Details | Diff | Splinter Review
30.93 KB, patch
Details | Diff | Splinter Review
594 bytes, patch
Details | Diff | Splinter Review
10.33 KB, patch
Details | Diff | Splinter Review
23.29 KB, patch
ted
: review+
Details | Diff | Splinter Review
34.91 KB, patch
ted
: review+
Details | Diff | Splinter Review
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.
Blocks: flake8
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.
Attachment #8599685 - Flags: review+
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
Attachment #8599685 - Flags: review+ → review-
Attached patch 1155969_E401.patch (obsolete) — Splinter Review
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?
Attachment #8599685 - Attachment is obsolete: true
Attachment #8601265 - Flags: review?(sledru)
(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.
Attachment #8601265 - Flags: review?(sledru)
Assigned for review to file owner. Removed modifications to .hgtags. Fixed flake8 E401 error: multiple imports on a single row
Attachment #8601265 - Attachment is obsolete: true
Attachment #8601891 - Flags: review?(wkocher)
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.
Attachment #8601891 - Flags: review?(wkocher) → review?(ted)
Attachment #8601891 - Flags: review?(ted) → review+
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?
(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.
Assignee: nobody → alexlee0227
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).
Attachment #8609121 - Flags: review?(ted)
Attachment #8609122 - Flags: review?(ted)
Attachment #8609123 - Flags: review?(ted)
Attachment #8609124 - Flags: review?(ted)
Attachment #8609125 - Flags: review?(ted)
Attachment #8609126 - Flags: review?(ted)
Attachment #8609128 - Flags: review?(ted)
Attachment #8609137 - Flags: review?(ted)
Attachment #8609138 - Flags: review?(ted)
Attachment #8609139 - Flags: review?(ted)
Attachment #8609140 - Flags: review?(ted)
Attachment #8609141 - Flags: review?(ted)
Attachment #8609142 - Flags: review?(ted)
Attachment #8609143 - Flags: review?(ted)
Attachment #8609144 - Flags: review?(ted)
Attachment #8609145 - Flags: review?(ted)
Attachment #8609146 - Flags: review?(ted)
(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 ;)
(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.
Attachment #8610322 - Flags: review?(ted)
Attachment #8610321 - Flags: review?(ted) → review+
Attachment #8610322 - Flags: review?(ted) → review+
Requesting the checkin-needed to land these patches asap (will have to be rebased otherwise)
Keywords: checkin-needed
Alex, could you remove the request for review which are no longer relevant? Thanks
Flags: needinfo?(alexlee0227)
https://hg.mozilla.org/mozilla-central/rev/3c2597bb3edb
https://hg.mozilla.org/mozilla-central/rev/9185ec7daf03
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Attachment #8609146 - Flags: review?(ted)
Attachment #8609145 - Flags: review?(ted)
Attachment #8609144 - Flags: review?(ted)
Attachment #8609143 - Flags: review?(ted)
Attachment #8609142 - Flags: review?(ted)
Attachment #8609141 - Flags: review?(ted)
Attachment #8609140 - Flags: review?(ted)
Attachment #8609139 - Flags: review?(ted)
Attachment #8609122 - Flags: review?(ted)
Attachment #8609123 - Flags: review?(ted)
Attachment #8609124 - Flags: review?(ted)
Attachment #8609125 - Flags: review?(ted)
Attachment #8609126 - Flags: review?(ted)
Attachment #8609128 - Flags: review?(ted)
Attachment #8609137 - Flags: review?(ted)
Attachment #8609138 - Flags: review?(ted)
Attachment #8609121 - Flags: review?(ted)
Flags: needinfo?(alexlee0227)
You need to log in before you can comment on or make changes to this bug.