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 C2CE71387A4 for ; Thu, 31 Jan 2013 18:39:25 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id 7B85921C005; Thu, 31 Jan 2013 18:39:24 +0000 (UTC) Received: from vms173011pub.verizon.net (vms173011pub.verizon.net [206.46.173.11]) by pigeon.gentoo.org (Postfix) with ESMTP id DE77C21C005 for ; Thu, 31 Jan 2013 18:39:23 +0000 (UTC) Received: from odin.tremily.us ([unknown] [72.68.84.219]) by vms173011.mailsrvcs.net (Sun Java(tm) System Messaging Server 7u2-7.02 32bit (built Apr 16 2009)) with ESMTPA id <0MHI003N46HM7M80@vms173011.mailsrvcs.net> for gentoo-catalyst@lists.gentoo.org; Thu, 31 Jan 2013 12:39:23 -0600 (CST) Received: by odin.tremily.us (Postfix, from userid 1000) id 4D42085F84F; Thu, 31 Jan 2013 13:39:22 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tremily.us; s=odin; t=1359657562; bh=Oh6fm1p2RNRIX4y+P7oeFN71w1zSci8BggKaP5c1UZY=; h=Date:From:To:Subject:References:In-Reply-To; b=kT563n8GR6sU7YarvLoOaKSda4UjVpBveIUI/JxghAVi9JC/EQ6MmXcNokcYCj6+v t0nwZVrU98Xpj7mXcEtkXEBKpml0w6L8fL7A5GimzGM/STWPDHq5lPj4yR7QRiEr4A Ey83af4AL88iasGB4Ca3N6fnNACjsSMHF3fC146E= Date: Thu, 31 Jan 2013 13:39:22 -0500 From: "W. Trevor King" To: gentoo-catalyst@lists.gentoo.org Subject: Re: [gentoo-catalyst] More proposed Catalyst changes Message-id: <20130131183922.GA3946@odin.tremily.us> References: <1357633976.4289.61.camel@big_daddy.dol-sen.ca> <1357980958.4289.82.camel@big_daddy.dol-sen.ca> 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=envbJBWh7q8WU6mo Content-disposition: inline In-reply-to: <1357980958.4289.82.camel@big_daddy.dol-sen.ca> OpenPGP: id=39A2F3FA2AB17E5D8764F388FC29BDCDF15F5BE8; url=http://tremily.us/pubkey.txt User-Agent: Mutt/1.5.21 (2010-09-15) X-Archives-Salt: 6734c3df-f350-4c28-a400-2362e0e3ae80 X-Archives-Hash: 0a9cd63c8b634552fd8ef73ec47465fd --envbJBWh7q8WU6mo Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Jan 12, 2013 at 12:55:58AM -0800, Brian Dolbec wrote: > You can get a git clone of my rewrite work at: > http://dev.gentoo.org/~dolsen/catalyst/ >=20 > rewrite branch Ah, it's good to see things looking more Pythonic in Catalyst-land ;). Here's my initial review to push this forward, although I'm not particularly well-versed in the Catalyst internals. This is branch is getting to the stage where rebasing will get annoying, so I think merging stuff that looks good into the master branch should happen sooner, rather than later ;). 321df3b whitespace cleanup This looks good, but there are also whitespace cleanups in some other commits (e.g. 2d36d29). I think we should just run something like: for FILE in $(git ls-tree -r --name-only HEAD | grep -v 'bz2$'); do sed -i 's/[[:space:]]*$//' "$FILE" done and have done with it ;). 38cd3bb add more configured defaults List the new settings (distdir, repo_name, packagedir, port_tmpdir, options, snapshot_name) in the commit message so I don't have to read the diff. Probably explain why you think they should be configurable as well. `options` is also a pretty ambiguous name. 016704a use the new configured snapshot_name and portdir settings Rather than a separate add (38cd3bb) and use (016704a), I think it would be better if new options had their addition and use rolled into a single commit (e.g. add snapshot_name and use it as one commit, add portdir and use it as another commit). This might also be a good place to move the path construction over to use os.path.join(). fefd501 use the portdir setting rather than hard-coded path Good to merge. 63a25eb Remove self.mounts and self.mountmap's use of paths for keys and pa= ths. Migrate more hardcoded paths to use settings. That's a long commit summary ;). Perhaps the =E2=80=9CMigrate=E2=80=A6=E2= =80=9D portion should go into the commit message body, or that could be split out into a separate commit. This might also be a good time to transition from `CatalystError, =E2=80=A6` to `CatalystError(=E2=80=A6)` where you're touching lines. I'm not sure which versions of Python Catalyst is trying to support, but I'd be happy to see the beginings of a migration to '{}'.format() for building strings (vs. the current `'' + ''` concatenation) 60919e4 Apply all Matt Turner's has_key() replacement patches This looks like a job for `rebase` ;). Cherry-picking will just lead to redundant commits. 7a909b9 cleanup long lines, improve useage() output formatting slightly Can we just switch from getopt to argparse (Python =E2=89=A52.7)? af6e6b7 Initial rearrangement of the python directories Hooray! 5eeb8b1 new minimal start script I think __version__ and __maintainer__ should live in catalyst/__init__.py. 267dbb6 add __init__.py's to modules and arch sub-pkgs Added with blank lines? 299e35d update the module loading paths for the new locations Can we just drop this import manipulation and use __all__? e26de08 chmod +x Squash into 5eeb8b1. 78d8be2 fix catalyst_support import to new location and specify imported mo= dules Looks good. 2d36d29 move catalyst_support, builder, catalyst_lock out of modules, into = the catalyst's base namespace Yay! There are some whitespace changes in here too though (see my comments on 321df3b). bc66f5d rename the modules subpkg to targets, it better reflects what it co= ntains Looks good. a7206bb rename files directory to etc to better reflect the directories con= tents Actually, files/ is also used for other things (e.g. built man pages, see MAN_PAGES in the Makefile). If we keep the rename to etc/, we might to just build the man pages under doc/. da6cbbf rename shell script targets directory to targets.sh so as to not be= confused with the new python targets directory =E2=80=9Cso as=E2=80=A6=E2=80=9D should go in the commit message body to ge= t a shorter summary. I'm not sold on this. Since they are data files intended to be used by the Python modules, why not move them to catalyst/targets/scripts [1]? f9f18be update gitignore The Scratch, catalystc, *.geany, and test* entries would appear to be specific to your local installation and usage. I'd drop them from the reroll. d23d96b revert urllib import from commit 64c16cae70da13de3c55d8555a2e4c5dcd= f2fcad since it is not used anywhere in catalyst 64c16cae does a lot more than import urllib, so I think =E2=80=9Crevert=E2= =80=9D gives the wrong impression. I'd remove the import entirely (instead of commenting it out), and say: catalyst/support.py: Remove unused urllib import This was introduced in commit 64c16cae70da13de3c55d8555a2e4c5dcdf2fcad but is no longer used. ab2c22a comment out some code introduced in commit b3475906d5f51a21ecaf4ff0= 48002a2f44face52 with an undefined variable "s". The code must always be h= itting the general except: pass block. Seems useless, if not maybe it'll s= pit out a true error, so the real problem can be fixed Multiple sentences in a commit summary! Also, take advantage of your version control and just rip the code out instead of commenting it out. I looked through b347590 (which is a 408-line monster for catalyst_support.py), but I'm not clear why Eric was messing with that code. At any rate, it looks like a good cantidate for replacement with subprocess.Popen, although see 1b2597e ;). 95704c7 fix an undefined variable Oops ;). I'd mention DESIRED_RLIMIT in the commit message. 1b2597e update todo for a spawn() cleanup. +lots I'll see if I can get through 968d818, =E2=80=A6 sometime later today. Cheers, Trevor [1]: http://docs.python.org/2/distutils/setupscript.html#installing-package= -data --=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 --envbJBWh7q8WU6mo Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (GNU/Linux) iQIcBAEBAgAGBQJRCrpYAAoJEEUbTsx0l5OMahwP+wX4omivv6ube5pPkxxFq55Q zvnZioAiupkeZZViA2iRhA+RpYiKcKue07MhkfKZydpaJHg27qfUy33Y/cs4/8S2 412oimL1Q0t1Z/57DOzU8PeYB1WdgA9aXWlm0Ll5OAv1J5+fZYxWM4xWevRdJRGN uqfx6GWLTfGKJw+4QuRxEM3/JBpsoi868Yn7/rrS5QIgSogbclINyBCHV+93Ib82 Mlq6rpJkZucZI2eRHh7nD74d3Fi3xjW9xsZ32I8agYJiO+sr/jRXPQUIlmH0PIzt Uk6pOSXlc7yTJCUvu1GB2vJp/uJ1pRcKqzWpHDVP9zGvlpXQrp8nrvc6jSHTwIWB zyqNS2zU1TH6QnT+vgMaNaS0pqCfXuLlRX4abgtEhyWHwRiwLvrq+Os4F8v6hftU luxeZE5c/Jui611Z1J7O4DJtqUiIA3xc3jCFY2mojsHc4Sq6GdQXuV+Cwur0Iqcz wR4zc48KCZ+XyCudTfzQ8k7/WAFqbm3VSB2omCj/aQ6qLH7YjLyjtdriYlGLasSF 2QG/i2QJeMS3XTm9/W2u4BxOVS0HXiPxw4MBOvGAOBztx1SKf42H1pkmuwMUqWan 2zqog92GvVhEtCWJBbC23K9lD92RCyB6pUNyPnr5UeMGjMNfv1zLWe/n4zRrHvZ5 jmL7riX/Oaak5ZJ40IXM =OjA1 -----END PGP SIGNATURE----- --envbJBWh7q8WU6mo--