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" <[email protected]> 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>
