From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from pigeon.gentoo.org ([69.77.167.62] helo=lists.gentoo.org) by finch.gentoo.org with esmtp (Exim 4.60) (envelope-from ) id 1LXVif-00023l-At for garchives@archives.gentoo.org; Thu, 12 Feb 2009 07:10:45 +0000 Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id 7CC5DE0497; Thu, 12 Feb 2009 07:10:44 +0000 (UTC) Received: from el-out-1112.google.com (el-out-1112.google.com [209.85.162.181]) by pigeon.gentoo.org (Postfix) with ESMTP id 59598E0497 for ; Thu, 12 Feb 2009 07:10:44 +0000 (UTC) Received: by el-out-1112.google.com with SMTP id b25so398344elf.1 for ; Wed, 11 Feb 2009 23:10:44 -0800 (PST) Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-portage-dev@lists.gentoo.org Reply-to: gentoo-portage-dev@lists.gentoo.org MIME-Version: 1.0 Sender: antarus@scriptkitty.com Received: by 10.142.82.6 with SMTP id f6mr310560wfb.182.1234422643179; Wed, 11 Feb 2009 23:10:43 -0800 (PST) In-Reply-To: References: Date: Wed, 11 Feb 2009 23:10:43 -0800 X-Google-Sender-Auth: e995daa981b8a17f Message-ID: Subject: Re: [gentoo-portage-dev] equery: RFC and code review From: Alec Warner To: gentoo-portage-dev@lists.gentoo.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Archives-Salt: 192bf750-5ed9-478b-ab3c-56c033df8e3a X-Archives-Hash: 9857d3a7af8027434fa2e326b20e78a2 On Tue, Feb 10, 2009 at 2:53 AM, Douglas Anderson 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 > >