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/
> 
> 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 paths. Migrate more hardcoded paths to use settings.

That's a long commit summary ;).  Perhaps the “Migrate…” 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, …`
to `CatalystError(…)` 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 ≥2.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 modules

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 contains

Looks good.

a7206bb rename files directory to etc to better reflect the directories contents

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

“so as…” should go in the commit message body to get 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 64c16cae70da13de3c55d8555a2e4c5dcdf2fcad since it is not used anywhere in catalyst

64c16cae does a lot more than import urllib, so I think “revert” 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 b3475906d5f51a21ecaf4ff048002a2f44face52 with an undefined variable "s".  The code must always be hitting the general except: pass block.  Seems useless, if not maybe it'll spit 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, … sometime later today.

Cheers,
Trevor

[1]: http://docs.python.org/2/distutils/setupscript.html#installing-package-data

-- 
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