Things that I've fixed have been removed.
Shawn Walker wrote:
Brock Pytlik wrote:
Webrev:
http://cr.opensolaris.org/~bpytlik/ips-7364-v1/
line 935: I can't help but think that variables referenced in the
comments should have ' or " around them to make it clear they aren't
part of the sentence's grammar. This applies to all of your changes
(not just this file or place).
I wasn't aware we were doing this. If we want to move to that standard
for doc strings, that's fine, but then the other doc strings should be
changed to match this.
line 992: i know this isn't a code review, but "prefix" in pub seems
likely to break once the dictionary compatibility layer that's in
place now for publisher objects is removed, maybe hasattr or just try
to get it's pub.prefix value and catch the AttributeError?
Please file a separate bug. Because this may come in very late, the only
thing I want to change in this wad is adding comments and reformatting
code. Depending on how late this lands, the reformatting code may get
removed as well so that only comments are added.
src/modules/client/query_parser.py:
line 46: insert a newline here after docstrings to separate them
from code or comments -- this applies everywhere, so I won't denote
the other cases (and other files)
line 134: s/effected/affected/ -- I think
Pretty sure it's effected.
http://www.merriam-webster.com/dictionary/effected has a good
explanation about the difference between the two.
src/modules/indexer.py:
line 213: s/axes/axis/ ?
Well, it's one axis, and two axes, from what I got from the dictionary.
src/modules/query_parser.py:
line 323: replace 'querier' with 'queryer' or 'caller' perhaps?
querier is the correct noun (according to m-w.com). I didn't want to put
caller because I believe querier is the more correct word. The function
which calls Query.init may not be the one who actually gets the results.
lines 1092-1136: can this be broken up into smaller pieces? 7 or 8
indent levels is a bit hard to follow :|
see previous comment about not changing code in this wad. And, in
general, no, this one I'm not particularly interested in breaking up.
I'd rather have 7 indent levels than the other options I've thought of
so far.
Cheers,
Thanks for taking a look, new webrev coming out shortly after I check a
couple of things then do the ever painful remerge.
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
