From: "W. Trevor King" <[email protected]>
On Sat, Dec 14, 2013 at 10:44:13AM -0800, Matt Turner wrote:
> I feel they're basically ready to go, but I want everyone to be in
> the habit of sending patches to the list, so please do.
Here you go :).
> Preemptive review: I saw just a few minor things, almost entirely
> dealing with lack of whitespace around operators. git log -p --reverse
> origin/master..wking/dolsen-rewrite-part-1 and then search for ^\+.*[^
> ]\+[^ ] (less regex) finds a couple.
I added spaces around = and +. Beyond that, I agree with Dustin:
On Sat, Dec 14, 2013 at 03:31:50PM -0600, Dustin C. Hatch wrote:
> On 12/14/2013 10:42, W. Trevor King wrote:
> > The current code base is so far from PEP 8 that changing touched
> > sections to match PEP 8 looks really out of place. In this
> > instance it's probably fine, but I'd ok with putting off all/most
> > PEP 8 cleanups to their own pure-reformatting commits.
>
> I strongly agree with this idea. If we start mixing PEP8 and non
> PEP8 compliant code, it will get really hard to read really fast. I
> think we should wait until most if not all of this rewrite get onto
> master before we start changing stuff for the sake of reformatting.
On Sat, Dec 14, 2013 at 10:44:13AM -0800, Matt Turner wrote:
> I'd opt to just put parenthesis around prints where we're going to be
> line wrapping its argument. It accomplishes the same thing as the
> trailing slash, but moves us slightly in the direction of python3. So,
> fix up lines you think it possible that match ^\+.*\\$
Whitespace inconsistency is still readable, but having ‘print(…)’
where we touch lines, surrounded by ‘print …’ where we didn't touch
lines is a bit too far for me ;). Can we just move to:
from __future__ import print_function
and ‘print(…)’ as a mass-replace once we finish the rest of Brian's
series?
> The other thing: I think we should drop most of the last patch for
> now. Wrapping long lines before we switch from 8-space tabs to PEP8
> 4-space style seems premature. The other half of that commit that
> replaces a series of prints with a single one looks good though.
I've split the commit and placed the long-line wrapping at the end of
this part of the series. There aren't too many lines in there, so
rebasing the remainder of the series onto master shouldn't be to bad
if you don't merge the last commit from this part.
> You may want to put your Signed-off-by on the patches to note that
> you refactored them heavily.
Done. Brian, I'll wait until you've acked this form of your branch,
and then add sign-offs from you too, so you can ok (or not) my Git
refactoring.
Cheers,
Trevor
Brian Dolbec (21):
modules/tinderbox_target.py: Use 'portdir' instead of hard-coding
'/usr/portage'
modules/generic_stage_target.py: Use 'portdir' instead of hard-coding
'/usr/portage'
modules/generic_stage_target.py: Use 'portdir' instead of hard-coding
'/usr/portage'
modules/generic_stage_target.py: Use 'distdir' instead of hard-coding
'${PORTAGE}/distfiles'
modules/generic_stage_target.py: Use a 'local_overlay' setting instead
of hard-coding '/usr/local/portage'
catalyst: Split confdefaults into line-per-entry
catalyst: Add 'repo_name' default
catalyst: Add 'snapshot_name' default
catalyst: Add 'packagedir' default instead of hard-coding
'/usr/portage/packages'
catalyst: Add 'port_tmpdir' default instead of hard-coding
'/var/tmp/portage'
modules/generic_stage_target.py: Don't use paths as mountmap keys
modules/generic_stage_target.py: Use 'proc' instead of '/proc' as the
mountmap key
modules/generic_stage_target.py: Use 'dev' instead of '/dev' as the
mountmap key
modules/generic_stage_target.py: Use 'distdir' instead of
'/usr/portage/distfiles' as the mountmap key
modules/generic_stage_target.py: Use 'port_tmpdir' instead of
'/var/tmp/portage' as the mountmap key
modules/generic_stage_target.py: Use 'devpts' instead of '/dev/pts' as
the mountmap key
modules/generic_stage_target.py: Use 'packagedir' instead of
'/usr/portage/packages' as the mountmap key
modules/generic_stage_target.py: Use 'kerncache' instead of
'/tmp/kerncache' as the mountmap key
modules/generic_stage_target.py: Use 'ccache' instead of
'/var/tmp/ccache' as the mountmap key
catalst: improve usage() output formatting slightly
catalyst: cleanup long lines
catalyst | 101 ++++++++++++++++++++++++----------------
modules/generic_stage_target.py | 92 ++++++++++++++++++------------------
modules/snapshot_target.py | 14 ++++--
modules/tinderbox_target.py | 8 +++-
4 files changed, 123 insertions(+), 92 deletions(-)
--
1.8.3.2
