On Thu, Feb 12, 2009 at 4:10 PM, Alec Warner <anta...@gentoo.org> wrote:
> I think one of the major things that annoyed me when reading the code
> is that you did add a new option parser...and you added it about a
> dozen times...(regexp used to basically find the start of the same
> code block over and over).
>
> anta...@kyoto ~/code/checkouts/genscripts-read-only/equery $ grep
> "opts = .x" *.py | wc -l
> 13
>
> Now the option parsing is quite complicated so I'm not sure if a
> complicated data structure to pass to a generic function is really a
> good use of ones time. But it stands out.
Actually, I think you'll find that the amount of duplicated opt
parsing code in each module is really minimal, essentially only that
one line that your regex matched. Each module has its own set of
options and handles those options differently, so putting it all in
__init__ would be a royal mess.
I can see how at a glance it would appear like duplicated code, but
check out the different between normal getopt and gnu_getopt and how I
have those working between __init__ and the modules. There's nothing
being parsed twice. If you can come up with a simpler, cleaner way,
write back with a little more detail. But I just reexamined it and
even tried moving some stuff around, and I think it's about as good as
it gets right now.
>
> None of your files has a copyright notice.
>
There's one in equery.py. The rest of equery will reside in
python/site-packages. Does each one need the same copyright notice? (I
honestly don't know the policy)
> I pray pp.print_error prints to stderr and not stdout; otherwise tends
> to be a big nitpick of mine when people write errors to stdout.
$ grep -A 2 'def print_error' /usr/lib/gentoolkit/pym/gentoolkit/pprinter.py
def print_error(s):
"""Prints an error string to stderr."""
sys.stderr.write(output.red("!!! ") + s + "\n")
Safe :)
> belongs.py has this gem:
> q.append(('^' if query[0] == '/' else '/') + re.escape(query) + '$')
>
> I realize it works; but honestly, not readable.
You're right, that's pretty nasty. But look what it came from!
q = map(lambda x: ((len(x) and x[0] == "/") and "^" or "/") +
re.escape(x) + "$", query)
But I agree that it can be further improved. How's this:
if query.startswith('/'):
query = "^%s$" % re.escape(query)
else:
query = "/%s$" % re.escape(query)
q.append(query)
> You use # FOO comments often; sometimes too often.
Yep, good point. I comment a little excessively, and believe it or not
emeta (now the meta module) was my first ever python script, so it had
lots of "obvious" comments. I'll try to get rid of the silly stuff as
I run into them again.
> in __init__.py your find_matches function is too long (180 lines or
> 4.5 of my screen heights). You've already split it up into convenient
> sections by comments (which are fine by the way) but you can probably
> make subroutines out of some of the subsections.
Good idea. I'll do that.
> in __init__.py you have InvalidRegex with a TODO to move it to another
> file. It seems like exceptions.py might be a worthy place.
Yep, not 100% done yet and modifying more blocks to use exceptions,
and then moving the exceptions out to exceptions.py is on my list of
TODOs.
> I think there are a number of cases where I'd prefer to see string
> interpolation instead of explicit addition:
>
> '%s/%s' % (cat, package)
>
> But thats just me being picky (and you use string interpolation often,
> which is good).
Explicit string catting is also one of my pet peeves, but py-2.6 is
getting a brand new string formatting syntax and "%" style
interpolation will be deprecated in 3k, so I only rewrote strings
where readability was suffering. I'm biding my time... but I
definitely agree with you here.
>
> Thats what I found in about 20 minutes of looking.
>
> -Alec
Muchly appreciated!
-Doug
