On Mon, Jan 23, 2012 at 12:39:26PM -0800, Brock Pytlik wrote: > On 01/21/12 00:26, Edward Pilatowicz wrote: > >On Fri, Jan 20, 2012 at 07:17:47PM -0800, Brock Pytlik wrote: > >>On 01/20/12 15:06, Edward Pilatowicz wrote:
<removed all the other things we converged on> > If it appeared that my main objection to removing the image as a > member of the PkgPlan is that it created too much delta, you have my > apologies for not being clearer. One of the standards I use when > deciding what kinds of changes to make to existing code is how much > mostly spurious change am I causing. If there's a good reason to > make the change it still happens, but it does make me stop and > reconsider what I have in mind. That was what my first email > probably reflected. > > If agreed with your goal of removing the image from the pkgplan > object, then I wouldn't have an issue with the amount of delta it > caused, unfortunately, I don't think that's the best way to achieve > what you want in the code. > > So I saw several issues presented as to why the image needs to be > removed from pkgplan: a) it makes serialization easier b) you > eventually want to store PkgPlan's as part of the PlanDescription > and pass them around independent of images. > > I think I've already addressed point a in my initial email. In the > webrev as it is today, you've replaced PkgPlan(image) with PkgPlan() > then two lines later > pp.somefunction_that_takes_an_image_as_an_argument. So I assert my > original suggestion still works. > > Now that I understand the larger picture though, I don't think the > right thing to do is to include PkgPlan's in the PlanDescription. > For starters, that would make the PkgPlan objects a part of the API > just like the publisher object is. In addition, it would mean i don't understand how including PkgPlan objects in the PlanDescription automatically makes them part of the API? currently the PlanDescription doesn't return pointers to PkgPlan objects and i wouldn't propose changing this. as long as PkgPlan objects are pointed to by a private pointer and not returned via a public member function, they remain a private implementation detail. (right now the PlanDescription has a private pointer to an imageplan object and we certainly don't consider that public.) > removing the image from the PkgPlan and passing it as a separate > argument to at least 10 different functions. To me, when something > is being used as an argument to that many functions, it's an > indication that it's probably something that should be part of the > object rather than a parameter to functions. Also, if you pass an looking at it another way, there are 26 public interfaces to the PkgPlan class (ignoring the 4 private interfaces), so about 38% of PkgPlan interfaces need this pointer. > image to functions independently, then that suggests that one could > pass a different image to evaluate than to download, which would > only cause headaches. > that argument i agree with. > Instead of removing the image parameter and making the changes > you've proposed, I suggest that we create a PkgPlanDescription > object, whose sole purpose is to describe a plan, but never to carry > out actions on an image. This allows clean separation of the > description of a plan for outside consumers (like the parent image > or displaying to the user) from the objects needed to carry out the > plan (which must have access and knowledge of the image to > manipulate). This is identical to the separation we made between the > ImagePlan and PlanDescription classes and I think the reasons we > made that split hold equally well for PkgPlan's and > PkgPlanDescriptions. > as stated above i don't think we want to make PkgPlan be part of the public API, hence i don't think that we need and/or want a PkgPlanDescription description object. that said, in the interest of making progress, how about i: - re-introduce it - make it optional (and default it to None) during PkgPlan initialization - expect callers who initialize a PkgPlan without an image pointer to set the image pointer before calling routines which require it. better? ed _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
