[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: 4369 bytes --]

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?”.

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 ;).

>  			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)):

> @@ -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().  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'.

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

[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

-- 
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 --]