On 07/27/11 23:33, Shawn Walker wrote:
On 07/22/11 14:25, Brock Pytlik wrote:
Webrev:
https://cr.opensolaris.org/action/browse/pkg/bpytlik/18439-v1
Bug:
18439 pkg commands that modify the image should have a parsable output
option
src/client.py:
lines 102-1111: This doesn't seem to include the license text?
It wasn't clear to me that having the license text was needed. If it
should be there, I'm happy to add it.
lines 1117-1129: I'd personally suggest naming these in terms of
what will be done instead of as if they had been already done. That
is, 'create-new-be' instead of 'new-be-created' (we also seem to be
missing be-name if one was specified). Or, 'activate-be' instead of
'be-activated'. I'd also suggest 'removed-packages' instead of
'removed-fmris' and so on. But these are mostly bike-shedding
suggestions.
Sure, I'm fine with that. I'll add one for the be name too, thanks for
pointing that out.
line 1106: shouldn't this be dest_tup = None?
Yep.
line 1130: Maybe add a comment that it's always None for parent
images or am I misunderstanding?
Sure, I can add a comment.
line 2521: should this be included in the if condition above?
doesn't seem like it should be output
It should be the else of the if just above it. You're right that it
shouldn't output anything.
src/modules/client/api.py:
line 866: missing space after 'True,'
lines 1026-1027: now we're yielding some sort of custom dictionary?
That's also not documented in the docstring, and why aren't we
returning some kind of PlanDescription object instead?
Sure I'll update the docstring. We're not returning a PlanDescription
object because Ed's doing a lot of work in that area and I saw no reason
to duplicate his efforts. I also saw no reason to go through the effort
of converting the dictionary into a PlanDescription object then back to
the exact same dictionary I'd just converted.
This will all need to change to reflect Ed's changes, but hopefully not
too substantially. In the interest of landing this in the appropriate
time, I made those decisions.
src/modules/client/imageplan.py:
line 244: docstring should be moved to just after 297 now, and a new
one added
Sure
src/tests/cli/t_pkg_revert.py:
line 132: s/C,/C, /
general: I'm going to assume that if --parsable is specified, there is
no other output from the client except JSON and that the
QuietProgressTracker is always used. I'm also going to assume
--parsable was intentionally not documented in in usage or pkg(1).
Yep on both counts.
I'm willing to consider documenting them, and if we ever do a
better/more general v1 parsable output, I'd probably be in favor of
documenting it. Since it's explicitly designed for a single consumer,
I'm inclined not to document it.
Brock
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
