1 |
On Sat, Jan 12, 2013 at 12:55:58AM -0800, Brian Dolbec wrote: |
2 |
> You can get a git clone of my rewrite work at: |
3 |
> http://dev.gentoo.org/~dolsen/catalyst/ |
4 |
> |
5 |
> rewrite branch |
6 |
|
7 |
Ah, it's good to see things looking more Pythonic in Catalyst-land ;). |
8 |
Here's my initial review to push this forward, although I'm not |
9 |
particularly well-versed in the Catalyst internals. This is branch is |
10 |
getting to the stage where rebasing will get annoying, so I think |
11 |
merging stuff that looks good into the master branch should happen |
12 |
sooner, rather than later ;). |
13 |
|
14 |
321df3b whitespace cleanup |
15 |
|
16 |
This looks good, but there are also whitespace cleanups in some other |
17 |
commits (e.g. 2d36d29). I think we should just run something like: |
18 |
|
19 |
for FILE in $(git ls-tree -r --name-only HEAD | grep -v 'bz2$'); do |
20 |
sed -i 's/[[:space:]]*$//' "$FILE" |
21 |
done |
22 |
|
23 |
and have done with it ;). |
24 |
|
25 |
38cd3bb add more configured defaults |
26 |
|
27 |
List the new settings (distdir, repo_name, packagedir, port_tmpdir, |
28 |
options, snapshot_name) in the commit message so I don't have to read |
29 |
the diff. Probably explain why you think they should be configurable |
30 |
as well. `options` is also a pretty ambiguous name. |
31 |
|
32 |
016704a use the new configured snapshot_name and portdir settings |
33 |
|
34 |
Rather than a separate add (38cd3bb) and use (016704a), I think it |
35 |
would be better if new options had their addition and use rolled into |
36 |
a single commit (e.g. add snapshot_name and use it as one commit, add |
37 |
portdir and use it as another commit). |
38 |
|
39 |
This might also be a good place to move the path construction over to |
40 |
use os.path.join(). |
41 |
|
42 |
fefd501 use the portdir setting rather than hard-coded path |
43 |
|
44 |
Good to merge. |
45 |
|
46 |
63a25eb Remove self.mounts and self.mountmap's use of paths for keys and paths. Migrate more hardcoded paths to use settings. |
47 |
|
48 |
That's a long commit summary ;). Perhaps the “Migrate…” portion |
49 |
should go into the commit message body, or that could be split out |
50 |
into a separate commit. |
51 |
|
52 |
This might also be a good time to transition from `CatalystError, …` |
53 |
to `CatalystError(…)` where you're touching lines. |
54 |
|
55 |
I'm not sure which versions of Python Catalyst is trying to support, |
56 |
but I'd be happy to see the beginings of a migration to '{}'.format() |
57 |
for building strings (vs. the current `'' + ''` concatenation) |
58 |
|
59 |
60919e4 Apply all Matt Turner's has_key() replacement patches |
60 |
|
61 |
This looks like a job for `rebase` ;). Cherry-picking will just lead |
62 |
to redundant commits. |
63 |
|
64 |
7a909b9 cleanup long lines, improve useage() output formatting slightly |
65 |
|
66 |
Can we just switch from getopt to argparse (Python ≥2.7)? |
67 |
|
68 |
af6e6b7 Initial rearrangement of the python directories |
69 |
|
70 |
Hooray! |
71 |
|
72 |
5eeb8b1 new minimal start script |
73 |
|
74 |
I think __version__ and __maintainer__ should live in |
75 |
catalyst/__init__.py. |
76 |
|
77 |
267dbb6 add __init__.py's to modules and arch sub-pkgs |
78 |
|
79 |
Added with blank lines? |
80 |
|
81 |
299e35d update the module loading paths for the new locations |
82 |
|
83 |
Can we just drop this import manipulation and use __all__? |
84 |
|
85 |
e26de08 chmod +x |
86 |
|
87 |
Squash into 5eeb8b1. |
88 |
|
89 |
78d8be2 fix catalyst_support import to new location and specify imported modules |
90 |
|
91 |
Looks good. |
92 |
|
93 |
2d36d29 move catalyst_support, builder, catalyst_lock out of modules, into the catalyst's base namespace |
94 |
|
95 |
Yay! There are some whitespace changes in here too though (see my |
96 |
comments on 321df3b). |
97 |
|
98 |
bc66f5d rename the modules subpkg to targets, it better reflects what it contains |
99 |
|
100 |
Looks good. |
101 |
|
102 |
a7206bb rename files directory to etc to better reflect the directories contents |
103 |
|
104 |
Actually, files/ is also used for other things (e.g. built man pages, |
105 |
see MAN_PAGES in the Makefile). If we keep the rename to etc/, we |
106 |
might to just build the man pages under doc/. |
107 |
|
108 |
da6cbbf rename shell script targets directory to targets.sh so as to not be confused with the new python targets directory |
109 |
|
110 |
“so as…” should go in the commit message body to get a shorter |
111 |
summary. |
112 |
|
113 |
I'm not sold on this. Since they are data files intended to be used |
114 |
by the Python modules, why not move them to catalyst/targets/scripts [1]? |
115 |
|
116 |
f9f18be update gitignore |
117 |
|
118 |
The Scratch, catalystc, *.geany, and test* entries would appear to be |
119 |
specific to your local installation and usage. I'd drop them from the |
120 |
reroll. |
121 |
|
122 |
d23d96b revert urllib import from commit 64c16cae70da13de3c55d8555a2e4c5dcdf2fcad since it is not used anywhere in catalyst |
123 |
|
124 |
64c16cae does a lot more than import urllib, so I think “revert” gives |
125 |
the wrong impression. I'd remove the import entirely (instead of |
126 |
commenting it out), and say: |
127 |
|
128 |
catalyst/support.py: Remove unused urllib import |
129 |
|
130 |
This was introduced in commit |
131 |
64c16cae70da13de3c55d8555a2e4c5dcdf2fcad but is no longer used. |
132 |
|
133 |
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 |
134 |
|
135 |
Multiple sentences in a commit summary! Also, take advantage of your |
136 |
version control and just rip the code out instead of commenting it |
137 |
out. I looked through b347590 (which is a 408-line monster for |
138 |
catalyst_support.py), but I'm not clear why Eric was messing with that |
139 |
code. At any rate, it looks like a good cantidate for replacement |
140 |
with subprocess.Popen, although see 1b2597e ;). |
141 |
|
142 |
95704c7 fix an undefined variable |
143 |
|
144 |
Oops ;). I'd mention DESIRED_RLIMIT in the commit message. |
145 |
|
146 |
1b2597e update todo for a spawn() cleanup. |
147 |
|
148 |
+lots |
149 |
|
150 |
I'll see if I can get through 968d818, … sometime later today. |
151 |
|
152 |
Cheers, |
153 |
Trevor |
154 |
|
155 |
[1]: http://docs.python.org/2/distutils/setupscript.html#installing-package-data |
156 |
|
157 |
-- |
158 |
This email may be signed or encrypted with GnuPG (http://www.gnupg.org). |
159 |
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy |