public inbox for gentoo-catalyst@lists.gentoo.org
 help / color / mirror / Atom feed
From: Brian Dolbec <dolsen@gentoo.org>
To: gentoo-catalyst@lists.gentoo.org
Subject: Re: [gentoo-catalyst] Re: [PATCH] Fix autoresume file paths to only be configured once.
Date: Tue, 4 Mar 2014 21:07:03 -0800	[thread overview]
Message-ID: <20140304210703.1fae17d0.dolsen@gentoo.org> (raw)
In-Reply-To: <20140305043528.GB25297@odin.tremily.us>

On Tue, 4 Mar 2014 20:35:28 -0800

Well for starters this patch was not part of the 3.0 branch.  When
working with patches in the pending branch I was running into some
troubles fixed in 3.0 that of course were going to be awhile before
coming to pending.  So when I started fixing a couple, I thought what
the hell, and did them all.  The unfortunate part is that a bunch of
these changes will be replaced by other code later in the patch series.



"W. Trevor King" <wking@tremily.us> wrote:

> 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.
> 
> > -		if "AUTORESUME" in self.settings\
> > -			and os.path.exists(self.settings["autoresume_path"]+\
> > -				"setup_target_path"):
> > +		setup_target_path_resume = pjoin(self.settings["autoresume_path"],
> > +			"setup_target_path")
> > +		if "AUTORESUME" in self.settings and \
> > +				os.path.exists(setup_target_path_resume):
> 
> How about:
> 
> 		setup_target_path_resume = os.path.join(
> 			self.settings["autoresume_path"],
> 			"setup_target_path")
> 		if ("AUTORESUME" in self.settings and
> 				os.path.exists(setup_target_path_resume)):
> 
> I don't think that's particularly unweildy, it avoids the need for
> line-continuation characters [1], and doesn't leave new readers
> wondering “what's pjoin?”.

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.


> 
> That's all minor stuff though.  I think the creation of a
> setup_target_path_resume variable is good.
> 
> > -		self.settings["autoresume_path"]=normpath(self.settings["storedir"]+\
> > -			"/tmp/"+self.settings["rel_type"]+"/"+".autoresume-"+\
> > -			self.settings["target"]+"-"+self.settings["subarch"]+"-"+\
> > -			self.settings["version_stamp"]+"/")
> > +		self.settings["autoresume_path"] = normpath(pjoin(
> > +			self.settings["storedir"], "tmp", self.settings["rel_type"],
> > +			".autoresume-%s-%s-%s"
> > +			%(self.settings["target"], self.settings["subarch"],
> > +				self.settings["version_stamp"])
> > +			))
> 
> +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.


> >  			if os.path.isdir(self.settings["source_path"]) \
> > -				and os.path.exists(self.settings["autoresume_path"]+"unpack"):
> > +				and os.path.exists(unpack_resume):
> 
> Another place where I'd follow PEP8 [1,2] and use:
> 
> 			if (os.path.isdir(self.settings["source_path"]) and
> 					and os.path.exists(unpack_resume)):
> 
> >  			elif os.path.isdir(self.settings["source_path"]) \
> > -				and not os.path.exists(self.settings["autoresume_path"]+\
> > -				"unpack"):
> > +				and not os.path.exists(unpack_resume):
> 
> I'd use:
> 
> 			elif (os.path.isdir(self.settings["source_path"]) and
> 					not os.path.exists(unpack_resume)):
> 

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.

Easy to fix...


> > @@ -876,9 +881,10 @@ class generic_stage_target(generic_target):
> >  				self.snapshot_lock_object.unlock()
> >  
> >  	def config_profile_link(self):
> > +		config_protect_link_resume = pjoin(self.settings["autoresume_path"],
> > +			"config_profile_link")
> >  		if "AUTORESUME" in self.settings \
> > -			and os.path.exists(self.settings["autoresume_path"]+\
> > -				"config_profile_link"):
> > +			and os.path.exists():
> 
> Oops, you dropped the argument from exists().

oops, missed that :/   hmm, why did I not discover that in testing...
I must not have run the right combination of things to discover it.

>  How about:
> 
> 	def config_profile_link(self):
> 		config_profile_link_resume = pjoin(
> 			self.settings['autoresume_path'], 'config_profile_link')
> 		if ('AUTORESUME' in self.settings and
> 				os.path.exists(config_profile_link_resume)):
> 
> I also switched 'protect' to 'profile' to match the method name, but I
> don't actually know what this file is marking ;).
> 
> > -			touch(self.settings["autoresume_path"]+"config_profile_link")
> > +			touch(config_protect_link_resume)
> 
> Here's another place I'd use 'profile' over 'protect'.

Yeah, My brain must have been stuck in "config_protect"  which is a
common portage variable and control.

> 
> There are a whole lot of these things with very boilerplate code.  Can
> we abstract this out to reduce that repetition?  I'm not sure what
> that would look like at the moment though, and a larger overhaul
> shouldn't stand in the way of this patch.
> 
> Cheers,
> Trevor

Like I said at the top of the reply.
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.


> 
> [1]: In PEP 8 [3]:
> 
>        The preferred way of wrapping long lines is by using Python's
>        implied line continuation inside parentheses, brackets and
>        braces. Long lines can be broken over multiple lines by
>        wrapping expressions in parentheses. These should be used in
>        preference to using a backslash for line continuation.
> 
> [2]: In PEP 8 [3]:
> 
>         The preferred place to break around a binary operator is after
>         the operator, not before it.
> 
> [3]: http://legacy.python.org/dev/peps/pep-0008/#maximum-line-length
> 



-- 
Brian Dolbec <dolsen>



  reply	other threads:[~2014-03-05  5:07 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-03-02 23:05 [gentoo-catalyst] [PATCH] Fix autoresume file paths to only be configured once Brian Dolbec
2014-03-05  4:35 ` [gentoo-catalyst] " W. Trevor King
2014-03-05  5:07   ` Brian Dolbec [this message]
2014-03-05  6:27     ` W. Trevor King
2014-03-23  0:19       ` Brian Dolbec

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140304210703.1fae17d0.dolsen@gentoo.org \
    --to=dolsen@gentoo.org \
    --cc=gentoo-catalyst@lists.gentoo.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox