Re: [gentoo-portage-dev] equery: RFC and code review

[Alec Warner]

On Tue, Feb 10, 2009 at 2:53 AM, Douglas Anderson <dja@××××××.com> wrote:
> Hi all,
>
> It's been a few months since we first discussed refactoring equery on
> this list and I think made pretty good time and I'm reasonably happy
> with it. So I'm throwing out to you all. I've tried to keep a running
> list of visible changes as I went along, but I'm sure I've missed some
> things. You can read that here:
>
> http://code.google.com/p/genscripts/source/browse/equery/trunk/equery/CHANGELOG
>
> My primary goals for the refactorization were:
> * Modularize
> * Clean up the UI
> * Rewrite a more solid option handler
> * Eliminate as much duplicated code as possible
> * Clean up slop and bitrot, follow clean coding standards and style
> guidelines wherever possible
> * Break up large functions, write docstrings for all functions
> * Get code set for Python 2.6 (tested in 2.5 and 2.6) and prepare
> smooth upgrade for Py3k.
> * Create a more consistent experience for the user.
>
> This last point is surely where the most contention will stem from, so
> let me argue my position. All modules except for list, check and size
> display the help message when called without input (ex: equery uses).
> This makes sense. It's what most other programs do. It's what emerge
> does. It's what eselect does. It's the neighborly thing to do.
>
> However these three exceptions go and start a time-consuming process
> which is of very limited usefulness. With a cold cache, each process
> takes at least 30 seconds. I imagine that 99 percent of people who
> type 'equery list' do it by accident. And who needs to see the size of
> every installed package? And checksum every installed package? Almost
> useless except to a select few people who know what they're doing.
>
> So I took the liberty of changing the default behavior of these three
> modules to be more in line with the rest of equery and the world. Each
> of the three displays deprecation warning explaining how to recreate
> the old default behavior. I can't imagine anyone objecting outright to
> this, considering there will always be more than one version of
> gentoolkit in the tree (there are 6 now) so it'd be a long time before
> anyone was forced into the new behavior.
>
> That's it. For anyone interested, please read the link above and let
> me know if you like it/hate it/want something else done.
>
> For python programmers, I'd appreciate a quick code review and any
> comments/criticism whatsoever (I really like criticism).

You Asked...

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

antarus@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.

None of your files has a copyright notice.

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.

belongs.py has this gem:
q.append(('^' if query[0] == '/' else '/') + re.escape(query) + '$')

I realize it works; but honestly, not readable.

append_str = ""
if query[0] == '/' append_str += '^' else append_str += '/'
append_str += re.escape(query) + '$'
q.append(append_str)

Slightly better (if append_str has a shorter name, maybe).  I don't
like 1 line if EXPR STATEMENT else STATEMENT but sometimes they are
shorter
Putting a conditional in the function call is horrible looking though.

You use # FOO comments often; sometimes too often.  Explaining what
you are doing is typically better noted in a docstring for a function
and not necessarily right next to the code it pertains to.  An example
would be meta.py in get_useflags.  You have a 4 line comment block
describing what you are doing but it really interferes with the
reading of the code.  Functions should typically be short enough that
referring to the docstring is no big deal (and the example I chose is
a very short function).  In addition you have a lot of "# do something
obvious here."  If it is obvious; you don't need a comment.  An
example of this would be meta.py around line 418.

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.  Also most of the
code has nothing to do with finding matches and lots to do with query
preparation.

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.

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

Thats what I found in about 20 minutes of looking.

-Alec

>
> Thanks in advance,
> -Doug
>
>