On Fri, Mar 06, 2009 at 05:55:52PM -0800, Brock Pytlik wrote:
> http://cr.opensolaris.org/~bpytlik/ips-2670-v1/
client.py:
- line 898: Do you actually need to call rstrip() here?
- I think you want to document what this function is doing. There
probably should be some public documentation of how set actions differ
from other actions in terms of search results, as well.
- line 900, 901: no need for parens around return statements
- line 952: the list here is identical to that in 964; perhaps that could
be done in a single place? I assume that because the remote copy
doesn't catch any exceptions, that it's not api.Query() that throws it,
but local_search() (and not remote_search()).
Should the None arguments passed in be defaults?
- line 976: Am I correct in reading this as a loop over one value? Why
not just do:
first = True
index, mfmri, action, value = tmp
retcode = 0
if first:
....
which of course raises the question of why you're bothering with
"first" there. So perhaps I'm reading the for loop wrong. Maybe "tmp"
needs a better name? "v" could, too (I assume it's the query version?).
- line 985: I think we can probably get rid of this by now -- servers
haven't been returning two-valued search results since before 2008.05
(bug 981).
- line 1019: "prefix" in auth is kinda weird. What is auth, exactly?
- line 1030, 1033: These exceptions were both caught above, but for local
searches only. Can this be made more consistent?
depend.py:
- line 265: why None for the second value, instead of "depend"?
directory.py:
- line 51: I think there are other places that might strip this ...
check. At any rate, this is bug 6062, right? If so, it needs to go
into the buglist.
- line 53: It's too late to raise a MalformedActionError here. It needs
to be done at the time the original string is parsed. Trying to
recreate that string with str(self) is impossible, as the order of
attributes in __str__() is arbitrary. There's an InvalidActionError
exception that's more appropriate.
- line 57: "Empty path attribute" probably should be put in _().
file.py
- line 61-63: Same comment here as in directory.py about the use of
MalformedActionError. In addition, it probably should be illegal to
have a trailing slash at the end of the pathname of a file.
- line 388: Seeing this reminded me of bug 1059, but I'm still not sure
of the answer there.
Though looking at that makes me think that at some point, search
results may need to include the image they belong to (if you're sitting
in your home directory, which is a user image, and you type "pkg
search", you may very well want to see results from the system image).
Whether this is an API issue or merely a display issue, I'm not sure.
generic.py:
- line 276: This obviously doesn't apply anymore. You *definitely* need
some documentation about what gets returned here now; it's not obvious
anymore, at least what the last element is, or why the first one is
necessary.
- line 290: "file" isn't always correct here -- license actions can have
data, too. Perhaps use self.name here instead?
group.py:
- line 110: why change "groupname" to "name"? Is it just because it's
redundant now that the action name is displayed to the user? Same
question for the user action.
license.py:
- line 149: No need for this comment -- it's not in the other concrete
action classes, and it's wrong in the same way that it is in
generic.py.
- line 163: When will this test be false? Is this a copy/paste job gone
bad?
link.py:
- line 50, 52: same issues here as for the file action.
- line 114, 116: use of action.name here could eliminate the need for
changing hardlink.py
api.py:
- line 905: I think Bart brought this up, but making this up on the fly
doesn't seem appropriate. There seem to be a lot of copies of this
scattered around.
- line 912: what about the rest of the loop?
- line 914: Definitely need a comment about the input and output of this
function. Same for local search. Good documentation is part of the
whole point of a public API.
- line 982, 998: I'd probably put these clauses into separate methods --
parse_v0_result() parse_v1_result(), maybe. Perhaps not here, I dunno.
That should help a lot with the indentation.
- line 1002: You shouldn't need the strip here -- split will ignore
leading and trailing spaces.
- line 1004: I don't think an assert() is appropriate here. If you get
fewer fields than you expect, raise an exception of some sort, or skip
the particular result, or something. Similarly, you might guard
against a bad server on line 1005, should it return a second field that
isn't an integer.
- line 1023: Ditto on the assert here.
- line 1032: Probably should give some indication of when you might want
to call this method.
api_errors.py:
- line 331: This isn't i18n'ed.
- line 353: I don't understand this.
- line 357: Ditch the space preceding "This".
- line 371: I've been wondering what "ac" and "pc" are, and I still don't
know. What's "c" stand for?
- line 381: Shouldn't the format string here be i18n'ed, too?
fmri.py:
- line 249: This should be "include_scheme".
manifest.py:
- line 365: Why did you rename this method?
- line 381: Shouldn't this be "set"?
- line 396: The key you're testing for doesn't match the key you're using
to modify the dictionary.
- line 403: Why not "for line in file_handle:"?
- line 404: Just strip(). No need to bendover().
- line 409: Is this comment missing an XXX? As it is, just let the
exception go uncaught.
- line 422,423: Why are you bothering?
query_parser.py:
- line 248: Couldn't this use encode_return_type()?
- line 706: four space indent.
- line 708: "in" should be on line with "for" if possible, otherwise
indented by four spaces.
- line 716: isn't this just .get_name()?
- line 722: where is this used?
- line 724: I don't think you need this.
client/query_parser.py:
- line 132,133: What does this do?
server/query_parser.py:
- Is there a particular reason you have a bunch of empty classes here?
Couldn't you just have imported the names directly into this module and
be done with it? You should also explain what you're doing in
QueryParser.__init__(), and why. Similarly for client/query_parser.py.
depot.py:
- line 235: It seems to me that this logic should be much lower down than
this.
- line 251: No spaces inside the quotes.
- line 253: I'd scrap this, put line 261 into the IndexError clause, and
lose the assert on line 263. Do you really want someone to cause a
server thread to assert simply by sending it a query with both params
and args? Prioritize one and ignore the other.
- line 265: At this point, you know that both args and params gave you
nothing, so you can simply say there was no query.
- line 289: I'm not sure what's going on here.
- line 294-296: Ditch the commented-out code.
- line 302: You used this construction before, so I'm not sure why you
didn't use it here ...
for i, (v, return_type, vals) in enumerate(res_list):
if a:
foo man chew
if b:
Fu Manchu
repository.py:
- line 335: multiline list comprehension (or generator expression)
format:
first line of text [
item
for loop or if clause
if clause or for loop
really long if clause or super verbose
for loop
]
Would yielding the results here help with memory consumption, or does
the consumer have to have the complete results?
search_storage.py:
- line 278: This can simply be a string. The one on line 308 can't,
since there's an empty string in the list, though I wonder if you
shouldn't just pass the first element of the list in as its own
argument.
- line 318, 325: couldn't these be combined into line 315?
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
