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 0B90A13873B for ; Wed, 5 Mar 2014 04:35:34 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id 8CCC7E09FD; Wed, 5 Mar 2014 04:35:31 +0000 (UTC) Received: from qmta14.westchester.pa.mail.comcast.net (qmta14.westchester.pa.mail.comcast.net [76.96.59.212]) by pigeon.gentoo.org (Postfix) with ESMTP id 020F5E09FD for ; Wed, 5 Mar 2014 04:35:30 +0000 (UTC) Received: from omta16.westchester.pa.mail.comcast.net ([76.96.62.88]) by qmta14.westchester.pa.mail.comcast.net with comcast id ZgTD1n0021uE5Es5EgbWTS; Wed, 05 Mar 2014 04:35:30 +0000 Received: from odin.tremily.us ([24.18.63.50]) by omta16.westchester.pa.mail.comcast.net with comcast id ZgbV1n00B152l3L3cgbVmX; Wed, 05 Mar 2014 04:35:30 +0000 Received: by odin.tremily.us (Postfix, from userid 1000) id DCE091068335; Tue, 4 Mar 2014 20:35:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tremily.us; s=odin; t=1393994128; bh=VIv6Rca0fgZ/SePJy+kTrYB5WIWN6NNCfUOXlckXjk0=; h=Date:From:To:Subject:References:In-Reply-To; b=PazyvcxftRtBFWWlMt1SqngUi3s7PqGNU9XoP2O/VyA93oltU2E2nH7PlyYFfy1WB sffA29ZhVd7J0av1hJvsJ6X2gse5iOEPdH/n/9otm9C8aSMkOMgLuXS6rGnU4N7ibT Z1a1KkxK2wekvgZWVA0r76IMpfQH70jAd8sVYmJE= Date: Tue, 4 Mar 2014 20:35:28 -0800 From: "W. Trevor King" To: gentoo-catalyst@lists.gentoo.org Subject: [gentoo-catalyst] Re: [PATCH] Fix autoresume file paths to only be configured once. Message-ID: <20140305043528.GB25297@odin.tremily.us> References: <1393801535-13921-1-git-send-email-dolsen@gentoo.org> 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: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="pvezYHf7grwyp3Bc" Content-Disposition: inline In-Reply-To: <1393801535-13921-1-git-send-email-dolsen@gentoo.org> OpenPGP: id=39A2F3FA2AB17E5D8764F388FC29BDCDF15F5BE8; url=http://tremily.us/pubkey.txt User-Agent: Mutt/1.5.22 (2013-10-16) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=comcast.net; s=q20121106; t=1393994130; bh=iHsHwBOepJW5PmF+uJE7DY4v6rqb9QyOhEF12Qldugk=; h=Received:Received:Received:Date:From:To:Subject:Message-ID: MIME-Version:Content-Type; b=BBXVElAOmc2MdejQPfkQw4Ib7YecQ7HmutvpK/zl/TEIud4JLZ/iWSFsGr3voyjKI QWtLkUDwbJZ/tPG60mHN3IBjANWIGZohmCo/iw6XXyKDwJGNfiIcmAOOoIqOu+/+Fa g3CZ9XaH658OqsybuswZxRjjK4RipBIaKz25U79f0+zVBR1/xlM1qkrcWEZZ5rG+Ff vEqjBzrDGP1FKNUPH34NAETc7dAnFCp0vBQ3IBhypbBlb4x/git5WxA6KOGARb8nMY uNx8WuXhsxySv8B4tbtwElvGX52sOb4DSgP3mmxMJXhopb59h6KIu7RYZxwGRn0YZ5 kxWxAm/l6hsVQ== X-Archives-Salt: 8c123f62-e990-4bba-90ed-168a155e8efc X-Archives-Hash: af71a5f40d96da974faec431d72fb47a --pvezYHf7grwyp3Bc Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 =3D 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 =3D 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 =E2=80=9Cwhat's pjoin?=E2=80=9D. That's all minor stuff though. I think the creation of a setup_target_path_resume variable is good. > - 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"]) > + )) +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() > =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(): Oops, you dropped the argument from exists(). How about: 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)): 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 --=20 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 --pvezYHf7grwyp3Bc Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJTFqmNAAoJEKKfehoaNkbtLe0QAIruj/md8jPbe9aGl1ib8Dq1 QfZ9q/Wq//N3KGuNaH7LLjpbrWIvsWUFXQzTYFy5+ErH8lk7jcuaF6ZH1vO8J8yQ J9a/1BO+/BrOSxENe3LZPPKRqlKTyUo/L4RbkXZgX+O7RFBjUIf2EalZkKd1mhUI 9qKSM8AX82sIRpHk3nOtfGotK73LXEXUQtC5iL6xfRLq1sdWNnhp143FFuxj+a8M irjuxQvdBwI9yyspPbxCRYhrVaZxL1uEEreBoUnXOPCAuXiWJZVzr0XgkcKADBk7 GvazbBUw4hMdpSmoGrI0u0MIgfiJbEMh17JuBD9FJdhu8l5Jl1kfNYU9scbWDz2O zG4sgRjcboVRu5jwiWKvVW52HOztdlPiKCJi+BXegg8HtdWhlBEcLO/PJ4/LbQvN 1uijYSOpiPWnKe4Oy9b7KzIQLKb2IjaMI8IuBZW7yE0Afj4iYtXkcn48U2sWxduq lab+R3GGCdYXUOeq1yLXkPFpLn4tnaiSsObj9tY8oGS1hxhTuyX4o7vWfTW6l28j F1WWfwZvD1G0Ox87n0CC5n9NR7YXsYdUa1gio9hUkNW3nVxs7VOrIBmd3Wy2CA7l kvstuS3mh2ioJgRUdnKLG+EJlU+qB9uHhshR0BYHGHScWbnzXnQ/OhCaNMyVcNW6 +jOw1TXkSoxwmqQXx2dO =4ayd -----END PGP SIGNATURE----- --pvezYHf7grwyp3Bc--