Danek Duvall wrote:
On Fri, Mar 06, 2009 at 05:55:52PM -0800, Brock Pytlik wrote:
http://cr.opensolaris.org/~bpytlik/ips-2670-v1/
I've removed things that I've fixed or agreed with.
client.py:
- line 898: Do you actually need to call rstrip() here?
Yep, otherwise I end up with:
INDEX ACTION VALUE PACKAGE
basename dir bin/example_dir
pkg:/[email protected]
basename file bin/example_path pkg:/[email protected]
fmri set example_pkg pkg:/[email protected]
Instead of:
INDEX ACTION VALUE PACKAGE
basename dir bin/example_dir pkg:/[email protected]
basename file bin/example_path pkg:/[email protected]
fmri set example_pkg pkg:/[email protected]
- 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.
Yep, I'll be adding documentation everywhere before I put back since
it's clear that shouild happen first.
- 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()).
Ok, I'll make a single large try block. Exceptions can bubble out from
the call to local_search, and the iteration of the results since
generators are being used. Also, both query creations can produce a
boolean query exception
Should the None arguments passed in be defaults?
Sure
- 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?).
Nope, you're right.
- line 1019: "prefix" in auth is kinda weird. What is auth, exactly?
It's (now) a publisher object. Prefix is the name to display to the user.
- line 1030, 1033: These exceptions were both caught above, but for local
searches only. Can this be made more consistent?
Made so that one try block wraps everything.
depend.py:
- line 265: why None for the second value, instead of "depend"?
Code higher up places "depend" in the second value. I can make the
change you specified if desired. Specifically the code in
search_new_dict 372-379 provides common logic across the action types.
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.
It's not consistently stripped, this seems like the safest place to make
sure it happens. Yep, sorry, forgot to add it to the bug number.
- 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.
Ok, i'll switch to an invalidActionError
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.
I'm not going to make the trailing slash check change in this wad.
- 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.
To summarize my conclusions from that other thread. There's no simple
solution so I'm going to keep the existing behavior for now, which means
continuing to prepend os.path.sep.
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.
Documentation will be coming before I put out the next webrev, probably
a couple of days from now.
- line 290: "file" isn't always correct here -- license actions can have
data, too. Perhaps use self.name here instead?
ok
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.
This is what the output looks like:
pkg search -l 'group::'
INDEX ACTION VALUE PACKAGE
name group mysql
pkg:/[email protected]
name group slocate pkg:/[email protected]
pkg search -l 'name:'
INDEX ACTION VALUE PACKAGE
name group mysql
pkg:/[email protected]
name user mysql
pkg:/[email protected]
name group slocate pkg:/[email protected]
name user svctag
pkg:/[email protected]
name user zfssnap
pkg:/[email protected]
I think that's what we want, hence the use of name. If it's not, I'll
switch to groupname.
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.
Um, I don't think Bart brought it up. Making what up on the fly doesn't
seem appropriate?
- line 912: what about the rest of the loop?
What? If there's no index dir, then something's gone horribly wrong
internally to the code.
- 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.
See previous comments about timing of documentation and that I'll now
add it and delay the put back for it.
- 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.
Ok
api_errors.py:
- line 371: I've been wondering what "ac" and "pc" are, and I still don't
know. What's "c" stand for?
Child, changed to action_child and package_child
fmri.py:
- line 249: This should be "include_scheme".
I don't understand why, since it's specifically adding "pkg", but since
you and shawn both seem to feel strongly about it I'll change it.
manifest.py:
- line 365: Why did you rename this method?
Because I was developing them in parallel for a time and never got back
around to switching it back. Changing now
- line 381: Shouldn't this be "set"?
Nope
- line 403: Why not "for line in file_handle:"?
Because that uses the iterator which doesn't play nice with .tell
query_parser.py:
- line 248: Couldn't this use encode_return_type()?
encode_return_type was a relic function
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.
See previous comments on documentation.
I probably could import the names directly. Didn't know I could do that.
I'll look into it.
depot.py:
- line 235: It seems to me that this logic should be much lower down than
this.
I disagree. Depot.py's job in my opinion is to translate the internal
data structures into a format for going over the wire. That's what it's
doing here.
- 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.
If the client is sending both, it's doing something wrong. The client
won't know how to map the query numbers to the queries correctly. I've
switched to raising a bad request response.
- line 265: At this point, you know that both args and params gave you
nothing, so you can simply say there was no query.
Isn't that what I'm doing? If you tried to do a search but did a search
on nothing, to me, that's a bad request.
- line 289: I'm not sure what's going on here.
When a simple query is sent, we need to be able to distinguish the no
results found case from the some results found case so that we can set
the response code appropriately. So, if there's only one generator in
the results list, we grab that generator and try to pull one item from
it. If fail, then the search had no results and we set the response
status to be no_content and exit. If we get a response, then we need to
continue on to the output function, but we need to stuff that answer we
got back onto the front of the generator. That's what the itertools call
is doing.
- 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
Because res is a generator function. res_list is a list of generator
functions
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?
See above comment. We're not constructing the entire result set in
memory, just constructing one generator function for each query that's
been made.
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.
I think a list of characters is clearer to me, but I'll change it if you
feel strongly. I like having the empty string in there as it means that
I don't have to special case for the first element in the list.
Danek
Thanks for taking a look.
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
