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
signature.asc
Description: OpenPGP digital signature
