| 1 |
On Sun, Mar 02, 2014 at 03:05:35PM -0800, Brian Dolbec wrote: |
| 2 |
> Use: pjoin as a shorter alias to os.path.join() |
| 3 |
|
| 4 |
I think the saved characters are not worth the mental overhead of an |
| 5 |
additional layer of indirection. |
| 6 |
|
| 7 |
> - if "AUTORESUME" in self.settings\ |
| 8 |
> - and os.path.exists(self.settings["autoresume_path"]+\ |
| 9 |
> - "setup_target_path"): |
| 10 |
> + setup_target_path_resume = pjoin(self.settings["autoresume_path"], |
| 11 |
> + "setup_target_path") |
| 12 |
> + if "AUTORESUME" in self.settings and \ |
| 13 |
> + os.path.exists(setup_target_path_resume): |
| 14 |
|
| 15 |
How about: |
| 16 |
|
| 17 |
setup_target_path_resume = os.path.join( |
| 18 |
self.settings["autoresume_path"], |
| 19 |
"setup_target_path") |
| 20 |
if ("AUTORESUME" in self.settings and |
| 21 |
os.path.exists(setup_target_path_resume)): |
| 22 |
|
| 23 |
I don't think that's particularly unweildy, it avoids the need for |
| 24 |
line-continuation characters [1], and doesn't leave new readers |
| 25 |
wondering “what's pjoin?”. |
| 26 |
|
| 27 |
That's all minor stuff though. I think the creation of a |
| 28 |
setup_target_path_resume variable is good. |
| 29 |
|
| 30 |
> - self.settings["autoresume_path"]=normpath(self.settings["storedir"]+\ |
| 31 |
> - "/tmp/"+self.settings["rel_type"]+"/"+".autoresume-"+\ |
| 32 |
> - self.settings["target"]+"-"+self.settings["subarch"]+"-"+\ |
| 33 |
> - self.settings["version_stamp"]+"/") |
| 34 |
> + self.settings["autoresume_path"] = normpath(pjoin( |
| 35 |
> + self.settings["storedir"], "tmp", self.settings["rel_type"], |
| 36 |
> + ".autoresume-%s-%s-%s" |
| 37 |
> + %(self.settings["target"], self.settings["subarch"], |
| 38 |
> + self.settings["version_stamp"]) |
| 39 |
> + )) |
| 40 |
|
| 41 |
+1. My personal preference is for: |
| 42 |
|
| 43 |
'.autoresume-{}-{}-{}'.format( |
| 44 |
self.settings['target'], |
| 45 |
self.settings['subarch'], |
| 46 |
self.settings['version_stamp']))) |
| 47 |
|
| 48 |
but whatever ;). |
| 49 |
|
| 50 |
> if os.path.isdir(self.settings["source_path"]) \ |
| 51 |
> - and os.path.exists(self.settings["autoresume_path"]+"unpack"): |
| 52 |
> + and os.path.exists(unpack_resume): |
| 53 |
|
| 54 |
Another place where I'd follow PEP8 [1,2] and use: |
| 55 |
|
| 56 |
if (os.path.isdir(self.settings["source_path"]) and |
| 57 |
and os.path.exists(unpack_resume)): |
| 58 |
|
| 59 |
> elif os.path.isdir(self.settings["source_path"]) \ |
| 60 |
> - and not os.path.exists(self.settings["autoresume_path"]+\ |
| 61 |
> - "unpack"): |
| 62 |
> + and not os.path.exists(unpack_resume): |
| 63 |
|
| 64 |
I'd use: |
| 65 |
|
| 66 |
elif (os.path.isdir(self.settings["source_path"]) and |
| 67 |
not os.path.exists(unpack_resume)): |
| 68 |
|
| 69 |
> @@ -876,9 +881,10 @@ class generic_stage_target(generic_target): |
| 70 |
> self.snapshot_lock_object.unlock() |
| 71 |
> |
| 72 |
> def config_profile_link(self): |
| 73 |
> + config_protect_link_resume = pjoin(self.settings["autoresume_path"], |
| 74 |
> + "config_profile_link") |
| 75 |
> if "AUTORESUME" in self.settings \ |
| 76 |
> - and os.path.exists(self.settings["autoresume_path"]+\ |
| 77 |
> - "config_profile_link"): |
| 78 |
> + and os.path.exists(): |
| 79 |
|
| 80 |
Oops, you dropped the argument from exists(). How about: |
| 81 |
|
| 82 |
def config_profile_link(self): |
| 83 |
config_profile_link_resume = pjoin( |
| 84 |
self.settings['autoresume_path'], 'config_profile_link') |
| 85 |
if ('AUTORESUME' in self.settings and |
| 86 |
os.path.exists(config_profile_link_resume)): |
| 87 |
|
| 88 |
I also switched 'protect' to 'profile' to match the method name, but I |
| 89 |
don't actually know what this file is marking ;). |
| 90 |
|
| 91 |
> - touch(self.settings["autoresume_path"]+"config_profile_link") |
| 92 |
> + touch(config_protect_link_resume) |
| 93 |
|
| 94 |
Here's another place I'd use 'profile' over 'protect'. |
| 95 |
|
| 96 |
There are a whole lot of these things with very boilerplate code. Can |
| 97 |
we abstract this out to reduce that repetition? I'm not sure what |
| 98 |
that would look like at the moment though, and a larger overhaul |
| 99 |
shouldn't stand in the way of this patch. |
| 100 |
|
| 101 |
Cheers, |
| 102 |
Trevor |
| 103 |
|
| 104 |
[1]: In PEP 8 [3]: |
| 105 |
|
| 106 |
The preferred way of wrapping long lines is by using Python's |
| 107 |
implied line continuation inside parentheses, brackets and |
| 108 |
braces. Long lines can be broken over multiple lines by |
| 109 |
wrapping expressions in parentheses. These should be used in |
| 110 |
preference to using a backslash for line continuation. |
| 111 |
|
| 112 |
[2]: In PEP 8 [3]: |
| 113 |
|
| 114 |
The preferred place to break around a binary operator is after |
| 115 |
the operator, not before it. |
| 116 |
|
| 117 |
[3]: http://legacy.python.org/dev/peps/pep-0008/#maximum-line-length |
| 118 |
|
| 119 |
-- |
| 120 |
This email may be signed or encrypted with GnuPG (http://www.gnupg.org). |
| 121 |
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy |
Archives