Danek Duvall wrote:
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?
It's not spaces, it's a newline. Try your example again with a "\n"
tacked onto the end of the string.
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.
Ok, sounds good.
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?
Ah, that was probably the issue. I was trying to look at the v1 webrev,
but kept finding myself looking at the current webrev (I have too many
tabs open to codereviews in my browser ;) ) Yeah, that's been fixed up :)
- 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.
Yeah, I see the issue now. I'll fix that tomorrow.
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)?
Yep.
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.
Ah, ok, changed to scheme.
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.
Ok, I'll fix that as well.
- 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.
Sure
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 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.
True.
- 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?
That's correct. For now, we lose observability when the client does a
multi-query search. If this is an issue, I think we can reexamine it.
Since this was mostly designed for publishing (or possibly other
programmatic use though what I can't think of right now), I thought this
was an ok tradeoff to make.
- 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
Ok, let me take a step back and try again. (Yes, I'll be adding comments
to this area since there seems to be a great deal of confusion about
what's going on.) Res_list isn't a list of results, or a generator which
produces results. It's a list of generators, one per query submitted.
Each iteration over a particular generator produces 3 things, the
version, the return type, and the values. (Yes, I could've done that
differently, but I tried to avoid stacking too many generators inside
generators as it just made my head hurt.)
To adapt your example, what I'd really want the output to be (roughly)
would be:
0 0
0 1
0 2
1 4
1 5
1 6
2 7
2 8
2 9
Does that make it clearer why I did things the way I did? The iteration
over res_list is iterating over queries. The iteration over res is
iterating over the results for that query.
Brock
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
