Re: [pkg-discuss] codereview request: info/list/status rework
On Thu, Apr 03, 2008 at 04:03:24PM -0700, Dan Price wrote: > > client.py: > > - line 635, 637: why not just do the str() once on line 612? > > By keeping the action an "action", the action comparitor gets used. So > if you print raw actions and sort by that column, you'll get the "right" > sort order. There's more we could do here-- I think if we write a > "columnizer" class we can flesh this idea out further. Makes sense. > > manifest.py: > > > > - line 224: why the assert? > > Ahh, because I now set self.size = 0. If you were to call > set_content multiple times, it would accumulate the added actions, > but the size would keep get reset to 0. > > I can rectify this either by renaming the routine "add_content" > and making associated fixes, or we can be more strict about it > being "set_content" and wipe out the actions and size previously > stored. Do you have a preference? I think that as set_content(), it should wipe out existing content. If we want to add content, we should have a separate method for that. You choose which you want to do. Or keep the assert for now, if you want. There's a lot of work that could be done here to make the API friendlier, but that's not this wad. Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] codereview request: info/list/status rework
On Thu 03 Apr 2008 at 12:34PM, Danek Duvall wrote: > On Thu, Apr 03, 2008 at 03:15:46AM -0700, Dan Price wrote: > > > http://cr.opensolaris.org/~dp/ips-cli-rewhack > > client.py: > - line 635, 637: why not just do the str() once on line 612? By keeping the action an "action", the action comparitor gets used. So if you print raw actions and sort by that column, you'll get the "right" sort order. There's more we could do here-- I think if we write a "columnizer" class we can flesh this idea out further. > manifest.py: > > - line 224: why the assert? Ahh, because I now set self.size = 0. If you were to call set_content multiple times, it would accumulate the added actions, but the size would keep get reset to 0. I can rectify this either by renaming the routine "add_content" and making associated fixes, or we can be more strict about it being "set_content" and wipe out the actions and size previously stored. Do you have a preference? In part the assert was there so I could see if we ever call set_content() more than once (and, we don't). I took the rest of your suggestions. -dp -- Daniel Price - Solaris Kernel Engineering - [EMAIL PROTECTED] - blogs.sun.com/dp ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] codereview request: info/list/status rework
On Thu, Apr 03, 2008 at 03:15:46AM -0700, Dan Price wrote:
> http://cr.opensolaris.org/~dp/ips-cli-rewhack
client.py:
- line 229: Feel free to say "because it keeps biting us", but why not
just "if not args"?
- line 230: Use "list()" to cast to a list. I might suggest just setting
"fmris = image.gen_installed_pkgs()", but you do make some small
assumptions that it's a list in the places that installed_fmris_from_args()
is called.
- line 505: from where the search wad was ripped out?
- line 561: "kB".
- line 572: is this comment out of date?
- line 635, 637: why not just do the str() once on line 612?
- line 659: This will no longer ever be true, right?
- line 680: you don't need this any more.
- line 719: would this be any clearer?
if set(("-H", "-o", "-t")).intersection(set([x[0] for x in opts])):
I would at least replace the map with a list comprehension.
- line 725: probably should be dedented, maybe moved up above the loop.
- line 729: are these really violations that should print out a usage
message? There isn't any indication in the usage message what the
valid special attributes are.
- line 764: space after comma
pkg.1.txt:
- line 13: not your bug, but could you change "pkh" to "pkg" here? And
since you're in this file, could you also fix bug 970?
- line 99: period at end of sentence.
- line 116: period instead of semicolon. Or don't capitalize next word
(and make it consistent with the next paragraph).
- line 194: is ":NAME" still used as the header?
manifest.py:
- line 224: why the assert?
- line 254: "self.size += int(action.attrs.get("pkg.size", "0"))"
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] codereview request: info/list/status rework
On Thu 03 Apr 2008 at 01:45PM, Albert White wrote: > > new # pkg info SUNWzone > > Name: SUNWzone > >Summary: Solaris Zones (Usr) > > Authority: foo (default) > >Version: 0.5.11 > > Build Release: 5.11 > > Branch: 0.79 > > Packaging Date: Tue Feb 5 17:23:10 2008 > > Size: 1016 KB > > FMRI: pkg:/[EMAIL PROTECTED],5.11-0.79:20080205T172310Z > > Just in terms of layout I think aligning things to the left looks better. eg. > > # pkg info SUNWzone > Name: SUNWzone > Summary: Solaris Zones (Usr) > Authority:foo (default) > Version: 0.5.11 > Build Release:5.11 > Branch: 0.79 > Packaging Date: Tue Feb 5 17:23:10 2008 > Size: 1016 KB I'm following a loose existing convention from commands such as coreadm, dumpadm, svcs -x. Personally I find the right alignment more aesthetic. It is not an interface, so we can always change it if people hate it. Even the old 'pkginfo -l' did it right-justified. > FMRI: pkg:/[EMAIL PROTECTED],5.11-0.79:20080205T172310Z > > Also should the repository from the FMRI not be listed here? If a user is > using multiple repositories it may be useful to know the repository the > package came from. I'll take a look. I thought it was doing things the way you suggested... perhaps when I merged I broke something. -dp -- Daniel Price - Solaris Kernel Engineering - [EMAIL PROTECTED] - blogs.sun.com/dp ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] codereview request: info/list/status rework
Dan,
> http://cr.opensolaris.org/~dp/ips-cli-rewhack
Mostly nits from me.
client.py:
172: I would re-write this as:
if not pkg.preferred_authority():
auth = " (" + pkg.get_authority() + ")"
else:
auth = ""
546: change (default) to (preferred)
Thanks for doing this.
-j
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] codereview request: info/list/status rework
> FMRI: pkg:/[EMAIL PROTECTED],5.11-0.79:20080205T172310Z > > Also should the repository from the FMRI not be listed here? The authority isn't printed in this case becacuse the package is installed from the preferred authority. If the package was installed from another authority, the FMRI could would automatically print the name of the authority in the FMRI. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] codereview request: info/list/status rework
Quoth Albert White on Thu, Apr 03, 2008 at 01:45:49PM -0700: > Dan Price wrote: > > new # pkg info SUNWzone > > Name: SUNWzone > >Summary: Solaris Zones (Usr) > > Authority: foo (default) > >Version: 0.5.11 > > Build Release: 5.11 > > Branch: 0.79 > > Packaging Date: Tue Feb 5 17:23:10 2008 > > Size: 1016 KB > > FMRI: pkg:/[EMAIL PROTECTED],5.11-0.79:20080205T172310Z > > Just in terms of layout I think aligning things to the left looks > better. eg. I agree it may be aesthetically better, but I'm afraid that if one of the labels becomes too long, it would be too hard to match the label with the data, so I prefer the right alignment. David ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] codereview request: info/list/status rework
Hi Dan, Dan Price wrote: > Here are some output comparisons: > -- > old # pkg info SUNWzone > Name: SUNWzone > FMRI: pkg://opensolaris.org/[EMAIL PROTECTED],5.11-0.79:20080205T172310Z > Version: 0.5.11 > Branch: 0.79 > Packaging Date: 2008-02-05 17:23:10 > Size: 1040836 > Summary: Solaris Zones (Usr) > > new # pkg info SUNWzone > Name: SUNWzone >Summary: Solaris Zones (Usr) > Authority: foo (default) >Version: 0.5.11 > Build Release: 5.11 > Branch: 0.79 > Packaging Date: Tue Feb 5 17:23:10 2008 > Size: 1016 KB > FMRI: pkg:/[EMAIL PROTECTED],5.11-0.79:20080205T172310Z Just in terms of layout I think aligning things to the left looks better. eg. # pkg info SUNWzone Name: SUNWzone Summary:Solaris Zones (Usr) Authority: foo (default) Version:0.5.11 Build Release: 5.11 Branch: 0.79 Packaging Date: Tue Feb 5 17:23:10 2008 Size: 1016 KB FMRI: pkg:/[EMAIL PROTECTED],5.11-0.79:20080205T172310Z Also should the repository from the FMRI not be listed here? If a user is using multiple repositories it may be useful to know the repository the package came from. Cheers, ~Al -- Albert White - Patch System Test - Sun Ireland ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
