On 01/23/12 16:24, Edward Pilatowicz wrote:
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.)
Sorry, I misunderstood what your intentions where here. I thought you
were exposing PkgPlan objects as something that consumers of a
PlanDescription object could access and use directly, if that's not the
case then you're correct they don't become part of the api.
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.
Sure, 38% seems like a large enough number to me.
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.
Cool.
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?
This sounds reasonable to me. I'd suggest perhaps making "image" a
property of a PkgPlan, rather than a public member, so that it can be
set if it's None, but not if it's anything else, that way a developer
couldn't accidentally change the image that a PkgPlan has been pointed
at (which would likely indicate some kind of bug in their code).
Brock
ed
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
