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

[Brian Dolbec]

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@×××××××.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>