From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lists.gentoo.org (pigeon.gentoo.org [208.92.234.80]) by finch.gentoo.org (Postfix) with ESMTP id F1E8E13873B for ; Wed, 5 Mar 2014 05:07:19 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id BE5EEE0999; Wed, 5 Mar 2014 05:07:15 +0000 (UTC) Received: from smtp.gentoo.org (smtp.gentoo.org [140.211.166.183]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by pigeon.gentoo.org (Postfix) with ESMTPS id 2FF52E0999 for ; Wed, 5 Mar 2014 05:07:15 +0000 (UTC) Received: from big_daddy.dol-sen.ca (S010600222de111ff.vc.shawcable.net [96.49.5.156]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: dolsen) by smtp.gentoo.org (Postfix) with ESMTPSA id 3808033FC48 for ; Wed, 5 Mar 2014 05:07:14 +0000 (UTC) Date: Tue, 4 Mar 2014 21:07:03 -0800 From: Brian Dolbec To: gentoo-catalyst@lists.gentoo.org Subject: Re: [gentoo-catalyst] Re: [PATCH] Fix autoresume file paths to only be configured once. Message-ID: <20140304210703.1fae17d0.dolsen@gentoo.org> In-Reply-To: <20140305043528.GB25297@odin.tremily.us> References: <1393801535-13921-1-git-send-email-dolsen@gentoo.org> <20140305043528.GB25297@odin.tremily.us> Organization: Gentoo Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-catalyst@lists.gentoo.org Reply-to: gentoo-catalyst@lists.gentoo.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Archives-Salt: 4d4ae8ed-68b3-4fdd-8635-0fae87beb3ac X-Archives-Hash: d15e48cc9df1bfe4519e4a70502b90ea 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" wrote: > On Sun, Mar 02, 2014 at 03:05:35PM -0800, Brian Dolbec wrote: > > Use: pjoin as a shorter alias to os.path.join() >=20 > I think the saved characters are not worth the mental overhead of an > additional layer of indirection. >=20 > > - if "AUTORESUME" in self.settings\ > > - and os.path.exists(self.settings["autoresume_path"]+\ > > - "setup_target_path"): > > + setup_target_path_resume =3D pjoin(self.settings["autoresume_path"], > > + "setup_target_path") > > + if "AUTORESUME" in self.settings and \ > > + os.path.exists(setup_target_path_resume): >=20 > How about: >=20 > setup_target_path_resume =3D os.path.join( > self.settings["autoresume_path"], > "setup_target_path") > if ("AUTORESUME" in self.settings and > os.path.exists(setup_target_path_resume)): >=20 > I don't think that's particularly unweildy, it avoids the need for > line-continuation characters [1], and doesn't leave new readers > wondering =E2=80=9Cwhat's pjoin?=E2=80=9D. 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. >=20 > That's all minor stuff though. I think the creation of a > setup_target_path_resume variable is good. >=20 > > - self.settings["autoresume_path"]=3Dnormpath(self.settings["storedir"= ]+\ > > - "/tmp/"+self.settings["rel_type"]+"/"+".autoresume-"+\ > > - self.settings["target"]+"-"+self.settings["subarch"]+"-"+\ > > - self.settings["version_stamp"]+"/") > > + self.settings["autoresume_path"] =3D normpath(pjoin( > > + self.settings["storedir"], "tmp", self.settings["rel_type"], > > + ".autoresume-%s-%s-%s" > > + %(self.settings["target"], self.settings["subarch"], > > + self.settings["version_stamp"]) > > + )) >=20 > +1. My personal preference is for: >=20 > '.autoresume-{}-{}-{}'.format( > self.settings['target'], > self.settings['subarch'], > self.settings['version_stamp']))) >=20 > but whatever ;). >=20 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): >=20 > Another place where I'd follow PEP8 [1,2] and use: >=20 > if (os.path.isdir(self.settings["source_path"]) and > and os.path.exists(unpack_resume)): >=20 > > 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): >=20 > I'd use: >=20 > elif (os.path.isdir(self.settings["source_path"]) and > not os.path.exists(unpack_resume)): >=20 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() > > =20 > > def config_profile_link(self): > > + config_protect_link_resume =3D 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(): >=20 > 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: >=20 > def config_profile_link(self): > config_profile_link_resume =3D pjoin( > self.settings['autoresume_path'], 'config_profile_link') > if ('AUTORESUME' in self.settings and > os.path.exists(config_profile_link_resume)): >=20 > I also switched 'protect' to 'profile' to match the method name, but I > don't actually know what this file is marking ;). >=20 > > - touch(self.settings["autoresume_path"]+"config_profile_link") > > + touch(config_protect_link_resume) >=20 > 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. >=20 > 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. >=20 > 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. >=20 > [1]: In PEP 8 [3]: >=20 > 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. >=20 > [2]: In PEP 8 [3]: >=20 > The preferred place to break around a binary operator is after > the operator, not before it. >=20 > [3]: http://legacy.python.org/dev/peps/pep-0008/#maximum-line-length >=20 --=20 Brian Dolbec