1 |
On Thu, Feb 12, 2009 at 4:10 PM, Alec Warner <antarus@g.o> wrote: |
2 |
> I think one of the major things that annoyed me when reading the code |
3 |
> is that you did add a new option parser...and you added it about a |
4 |
> dozen times...(regexp used to basically find the start of the same |
5 |
> code block over and over). |
6 |
> |
7 |
> antarus@kyoto ~/code/checkouts/genscripts-read-only/equery $ grep |
8 |
> "opts = .x" *.py | wc -l |
9 |
> 13 |
10 |
> |
11 |
> Now the option parsing is quite complicated so I'm not sure if a |
12 |
> complicated data structure to pass to a generic function is really a |
13 |
> good use of ones time. But it stands out. |
14 |
|
15 |
Actually, I think you'll find that the amount of duplicated opt |
16 |
parsing code in each module is really minimal, essentially only that |
17 |
one line that your regex matched. Each module has its own set of |
18 |
options and handles those options differently, so putting it all in |
19 |
__init__ would be a royal mess. |
20 |
|
21 |
I can see how at a glance it would appear like duplicated code, but |
22 |
check out the different between normal getopt and gnu_getopt and how I |
23 |
have those working between __init__ and the modules. There's nothing |
24 |
being parsed twice. If you can come up with a simpler, cleaner way, |
25 |
write back with a little more detail. But I just reexamined it and |
26 |
even tried moving some stuff around, and I think it's about as good as |
27 |
it gets right now. |
28 |
|
29 |
> |
30 |
> None of your files has a copyright notice. |
31 |
> |
32 |
|
33 |
There's one in equery.py. The rest of equery will reside in |
34 |
python/site-packages. Does each one need the same copyright notice? (I |
35 |
honestly don't know the policy) |
36 |
|
37 |
> I pray pp.print_error prints to stderr and not stdout; otherwise tends |
38 |
> to be a big nitpick of mine when people write errors to stdout. |
39 |
|
40 |
$ grep -A 2 'def print_error' /usr/lib/gentoolkit/pym/gentoolkit/pprinter.py |
41 |
def print_error(s): |
42 |
"""Prints an error string to stderr.""" |
43 |
sys.stderr.write(output.red("!!! ") + s + "\n") |
44 |
|
45 |
Safe :) |
46 |
|
47 |
> belongs.py has this gem: |
48 |
> q.append(('^' if query[0] == '/' else '/') + re.escape(query) + '$') |
49 |
> |
50 |
> I realize it works; but honestly, not readable. |
51 |
|
52 |
You're right, that's pretty nasty. But look what it came from! |
53 |
|
54 |
q = map(lambda x: ((len(x) and x[0] == "/") and "^" or "/") + |
55 |
re.escape(x) + "$", query) |
56 |
|
57 |
But I agree that it can be further improved. How's this: |
58 |
|
59 |
if query.startswith('/'): |
60 |
query = "^%s$" % re.escape(query) |
61 |
else: |
62 |
query = "/%s$" % re.escape(query) |
63 |
q.append(query) |
64 |
|
65 |
> You use # FOO comments often; sometimes too often. |
66 |
|
67 |
Yep, good point. I comment a little excessively, and believe it or not |
68 |
emeta (now the meta module) was my first ever python script, so it had |
69 |
lots of "obvious" comments. I'll try to get rid of the silly stuff as |
70 |
I run into them again. |
71 |
|
72 |
> in __init__.py your find_matches function is too long (180 lines or |
73 |
> 4.5 of my screen heights). You've already split it up into convenient |
74 |
> sections by comments (which are fine by the way) but you can probably |
75 |
> make subroutines out of some of the subsections. |
76 |
|
77 |
Good idea. I'll do that. |
78 |
|
79 |
> in __init__.py you have InvalidRegex with a TODO to move it to another |
80 |
> file. It seems like exceptions.py might be a worthy place. |
81 |
|
82 |
Yep, not 100% done yet and modifying more blocks to use exceptions, |
83 |
and then moving the exceptions out to exceptions.py is on my list of |
84 |
TODOs. |
85 |
|
86 |
> I think there are a number of cases where I'd prefer to see string |
87 |
> interpolation instead of explicit addition: |
88 |
> |
89 |
> '%s/%s' % (cat, package) |
90 |
> |
91 |
> But thats just me being picky (and you use string interpolation often, |
92 |
> which is good). |
93 |
|
94 |
Explicit string catting is also one of my pet peeves, but py-2.6 is |
95 |
getting a brand new string formatting syntax and "%" style |
96 |
interpolation will be deprecated in 3k, so I only rewrote strings |
97 |
where readability was suffering. I'm biding my time... but I |
98 |
definitely agree with you here. |
99 |
|
100 |
> |
101 |
> Thats what I found in about 20 minutes of looking. |
102 |
> |
103 |
> -Alec |
104 |
|
105 |
Muchly appreciated! |
106 |
-Doug |