Shawn Walker wrote: > On 07/26/11 13:48, Danek Duvall wrote: > >Shawn Walker wrote: > > > >> https://cr.opensolaris.org/action/browse/pkg/swalker/pkg-image-1/webrev/ > > > >pkg.1.txt: > > > > - line 147: "... rooted at directory dir. If no directory was ..." > > > >client.py: > > > > - line 5819: I think I understand why you did it this way, but it seems > > very strange to me to set pkg_image_used based on the environment > > variable, rather than on a return value of get_default_image_root(). > > The way it is, it assumes a) that get_default_image_root() uses > > PKG_IMAGE (which may be part of the method contract) and b) it hasn't > > been removed from the environment since the call (which won't happen in > > our code, but isn't impossible for other clients). > > a) is part of the method contract, and b) yes, it's assuming it > hasn't been removed from environment. Is it sufficient to add a > comment for that? > > My initial implementation actually returned the source as a string > token (e.g. "PKG_IMAGE", "CWD", or "PLATFORM") but I wasn't > comfortable making that "interface".
Yeah, I can't say I'm happier with that, either. A comment is probably fine. > > > >api.py: > > > > - line 4639: "may not actually be valid" probably needs a touch more > > explanation. Why should a client use an invalid root as a default > > location? Would they be expected to do an upwards search, or is it > > that find_root() will simply handle that? (If so, we might mention > > that it -- or the Image constructor -- will correct it.) > > find_root() handles that, I've expanded the docstring there with this: > > The ImageInterface object will use the root provided as a starting point > to find an image, searching upwards through each parent directory until > '/' is reached. The '/' may not be a valid image on systems such as > Solaris 10. Sure. > ... > > - line 4652: you can exit this if clause with root still None. Should it > > just be set to the live root at that point? > > As far as I can see, that only happens if portable.osname != "sunos", > which was my intention per comments on lines 4663-4666. So what happens if None is passed to find_root()? If None is supposed to be a return value, then it should be noted in the docstring, along with what that means semantically. Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
