Closed
Bug 1155969
Opened 10 years ago
Closed 10 years ago
xpcom/typelib/ code should follow the flake8 convention
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: Sylvestre, Assigned: alexlee0227, Mentored)
References
Details
(Whiteboard: [good first bug][lang=Python])
Attachments
(20 files, 2 obsolete files)
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.
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+
Reporter | ||
Comment 2•10 years ago
|
||
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-
Comment 3•10 years ago
|
||
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
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)
Reporter | ||
Comment 5•10 years ago
|
||
(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.
Reporter | ||
Updated•10 years ago
|
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)
Updated•10 years ago
|
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?
Reporter | ||
Comment 9•10 years ago
|
||
(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
Comment 10•10 years ago
|
||
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).
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8609121 -
Flags: review?(ted)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8609122 -
Flags: review?(ted)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8609123 -
Flags: review?(ted)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8609124 -
Flags: review?(ted)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8609125 -
Flags: review?(ted)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8609126 -
Flags: review?(ted)
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8609128 -
Flags: review?(ted)
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8609137 -
Flags: review?(ted)
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8609138 -
Flags: review?(ted)
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8609139 -
Flags: review?(ted)
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8609140 -
Flags: review?(ted)
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8609141 -
Flags: review?(ted)
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8609142 -
Flags: review?(ted)
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8609143 -
Flags: review?(ted)
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8609144 -
Flags: review?(ted)
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #8609145 -
Flags: review?(ted)
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8609146 -
Flags: review?(ted)
Reporter | ||
Comment 28•10 years ago
|
||
(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 ;)
Assignee | ||
Comment 29•10 years ago
|
||
(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.
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8610321 -
Flags: review?(ted)
Assignee | ||
Comment 31•10 years ago
|
||
Attachment #8610322 -
Flags: review?(ted)
Updated•10 years ago
|
Attachment #8610321 -
Flags: review?(ted) → review+
Updated•10 years ago
|
Attachment #8610322 -
Flags: review?(ted) → review+
Reporter | ||
Comment 32•10 years ago
|
||
Requesting the checkin-needed to land these patches asap (will have to be rebased otherwise)
Keywords: checkin-needed
Reporter | ||
Comment 33•10 years ago
|
||
Alex, could you remove the request for review which are no longer relevant? Thanks
Flags: needinfo?(alexlee0227)
Comment 34•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c2597bb3edb
https://hg.mozilla.org/integration/mozilla-inbound/rev/9185ec7daf03
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3c2597bb3edb
https://hg.mozilla.org/mozilla-central/rev/9185ec7daf03
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
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)
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(alexlee0227)
You need to log in
before you can comment on or make changes to this bug.
Description
•