[gentoo-catalyst] Re: [PATCH] Fix autoresume file paths to only be configured once.

["W. Trevor King" <wking@tremily.us>]

[-- Attachment #1: Type: text/plain, Size: 2788 bytes --]

On Tue, Mar 04, 2014 at 09:07:03PM -0800, Brian Dolbec wrote:
> The unfortunate part is that a bunch of these changes will be
> replaced by other code later in the patch series.

I don't think there are any critical fixes here.  If it's easier to
drop this cleanup and just jump straight to the final version, that's
fine with me too.

> > On Sun, Mar 02, 2014 at 03:05:35PM -0800, Brian Dolbec wrote:
> > > Use: pjoin as a shorter alias to os.path.join()
> > 
> > I think the saved characters are not worth the mental overhead of
> > an additional layer of indirection.
> > …
> 
> pjoin is something from snakeoil too, depending on the python
> version being run, it does some optimization changes if I remember
> correctly.  But yes, it is a minor change.

I don't see anything in the their docs claiming optimizations [1], but
they do have a C implementation [2], so there must be something they
don't like about os.path.join.  I doubt any optimizations would matter
to us, since Python-side path joins are going to be a miniscule part
of any catalyst run.

> > +1.  My personal preference is for:
> > 
> > 			'.autoresume-{}-{}-{}'.format(
> > 				self.settings['target'],
> > 				self.settings['subarch'],
> > 				self.settings['version_stamp'])))
> > 
> > but whatever ;).
> 
> I never think of the new .format method.  My brain is used to %s, so
> is stuck there.

I don't think it really matters.

> To be honest, I never thought about that during these changes, I was
> just doing what was needed to fix some old issues I had fixed
> differently in 3.0, but were going to be awhile.

Which issues?  Just assigning the paths to new variables?  I'm fine
with this commit doing that, regardless of style, and will happily
submit any PEP 8 cleanups I think we need after master has finished
swallowing 3.0 ;).

> The autoresume stuff is later replaced with a self contained class.
> I was never sure if I should submit it, but it did fix a couple
> small issues.  I can go either way with this one.

Pulling it out into a class sounds good to me, but if it's easier to
land this first (after fixing the exists() typo) and submit the
class-based autoresume later, I'm fine with that.  I wanted to point
out possible style changes, but none of that needs to be settled by
this particular patch.  I think the removal of duplicate path
calculations is a step in the right direction.

Cheers,
Trevor

[1]: http://docs.snakeoil.googlecode.com/git/api/snakeoil.osutils.html#snakeoil.osutils.pjoin
[2]: https://code.google.com/p/snakeoil/source/browse/src/posix.c#127

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]