On Tue, Mar 10, 2009 at 06:22:34PM -0700, Brock Pytlik wrote:
> 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]
How, exactly? Spaces don't ever end up in an attribute value unless
they're quoted, so stripping spaces from the action string shouldn't affect
the attribute values at all:
>>> a = pkg.actions.fromstr("dir path=usr/bin ")
>>> a.attrs["path"]
'usr/bin'
>>>
How do you create the example?
>> 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.
This is the only place, though, that you put None in this particular field.
It seems out of place that way to me, but I know I got a bit confused
trying to tie all these bits together without any explanation of what was
supposed to be happening. :) Once you've put comments and docstrings in,
I'll go back and check some of this over again.
>> 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.
Huh. Okay.
>> 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?
The exclude list -- you're always consing up one that just has the
architecture in it, but that might not be the only one.
My line numbers are all off of your v1 webrev -- might you be looking at
newer code?
>> - 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.
Yeah, I'm pretty sure you're looking at the wrong code. This is at the
tail end of local_search(), where you unconditionally return at the end of
the loop body. Since you never continue in the loop, you always return
after one body execution.
>> 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
Child, as in child query (ie, a side of a boolean)?
>> 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.
It's confusing, because there's no way to distinguish from the string "pkg"
and the contraction of "package" "pkg". I read this as "include package",
which is weird.
>> 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
Ah. So why did you choose the string "attribute" when the name of the
action is "set"? I see where it's coming from, but I think it should
mirror action.name, which is where that field originally comes from.
>> - line 403: Why not "for line in file_handle:"?
>>
> Because that uses the iterator which doesn't play nice with .tell
Ah, okay. Please comment.
>> 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.
Hm, okay.
>> - 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.
Or malicious.
> The client won't know how to map the query numbers to the queries
> correctly. I've switched to raising a bad request response.
That ought to do, thanks.
>> - 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.
Okay, but you also know that args and params are both empty, so printing
them out doesn't seem terribly useful.
>> - 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.
Gotcha. Comment, please.
Does this mean that we don't return a 204 when none of the queries in a
multi-query search return any results?
>> - 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
And that makes it not work?
>>> l = [xrange(3), xrange(4, 7), xrange(7, 10)]
>>> for i, (a, b, c) in enumerate(l):
... print i, a, b, c
...
0 0 1 2
1 4 5 6
2 7 8 9
>> 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.
As far as I can tell, you're already treating it completely separately --
you always look at split_chars[0] or split_chars[1:]. Same with
unquote_list.
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
