On 08/08/11 11:21, Edward Pilatowicz wrote:
On Sun, Aug 07, 2011 at 07:39:33PM -0700, Brock Pytlik wrote:
On 08/05/11 16:32, Edward Pilatowicz wrote:
On Tue, Aug 02, 2011 at 06:37:22PM -0700, Brock Pytlik wrote:
On 07/22/11 14:25, Brock Pytlik wrote:
Webrev:
https://cr.opensolaris.org/action/browse/pkg/bpytlik/18439-v1
New webrev:
https://cr.opensolaris.org/action/browse/pkg/bpytlik/18439-v2
sorry for the delay.
i still need to review the tests.
comments on the code are below.
ed
----------
general
- i thought that the last time we talked about this we had agreed on
putting a copy of the "parsable_version" into the progress tracker
class? if we did this we wouldn't have to add an extra parameter to
all the linked image functions, and we wouldn't have to scan the
arguments in __pkg_cmd().
I don't remember agreeing on that, I remember discussing it. When I
got back and looked at the code more, I decided that I didn't like
it inside the progress tracker since it doesn't have anything to do
w/ the ProgressTracker, and just stuffing it inside didn't strike me
as right.
we've got verbose and quiet in there, so i think parsable fits pretty
well, i also think it simplify the code a bunch because you wouldn't
need to add new parameters to so many functions.
I have to agree with Brock; it makes little sense to me that have this
in ProgressTracker. It isn't for monitoring progress -- it's purely a
parsable version of the plan.
...
hm. i was thinking that json is a generic output agnostic format. the
fact that the json is being emitted to stdout is a detail of the client,
but the json data itself and it's format seemed more directly related to
the plan description logic itself. but if other folks don't want this
as part of the plan description object then i'm ok with leaving it as
part of the client.
Brock and I disagreed as well on this particular point, but since this
is undocumented functionality for a specific consumer, I'm now indifferent.
...
- in display_plan(), if we're in parsable output mode we don't display
any licenses or mark them as installed. is this correct? if so it
seems like parsable output mode can't actually be used to install
packages? if so, perhaps -n should be required for parsable output
mode?
No. We include the licenses and text in the parsable output.
well, perhaps i'm reading the code wrong, but it seems to me that since
you don't call display_plan_licenses(), you also don't call
set_plan_license_status(), so any licenses that requires acceptance
won't be marked as being accepted, so executing any plan that requires
license acceptance will fail in parsable output mode.
Seems right.
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
