Exemplar configuration thoughts

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Exemplar configuration thoughts

Eric Crahen-3
I have a lot clearer picture of what I think might be needed to reduce the amount of per-project configuration required when many projects would use the same plugin with the same or almost identical configuration. I've been doing some experimentation and I've found a way to do what I'd like to do, but I think we can make some big improvements here. Some of it some of what you've said earlier. I thought it might be better all collected in an appropriately named post since I've come to this all in a roundabout way.

First, I'll just briefly mention something about the global configuration. Terminology sometimes differes between people and is the source for a lot of confusion. The notion of global configuration we have right now is that its for settings that are orthogonal to the actual settings a Project would be interested in. For example,

- The Ant task stores a global configuration in its Descriptor singleton. The values it stores are just the names and locations of the Ant installations available on the system. This configuration literally consists of names and paths. The Project configuration for the Ant task is stored in the instances of the Ant Builder member variables. The configuration for this component is not paths and names, its a reference to to the AntLocation to use.

In this example, the global configuration is a definition, the project configuration values are a selection. Related, but orthogonal. This works very well for the sort of thing that Ant configures.

The notion of global configuration I have been looking for is that of an exemplar global.

- There is a global instance of an ExtensionPoint and configure it as an exemplar. The configuration is actually stored in an ExtensionPoint instance, and not in the Descriptor.
- My global.jelly and config.jelly would look very similar, in fact they could be identical - the only difference is the instance of the ExtensionPoint being configured.

This might seem similar because the word global keeps popping up (but thats what this clarification is about) The subtle difference here is that, the global configuration in this case is not orthogonal to the project configuration. It actually is the same thing.

---

Now here are the challenges that I've been facing trying to come up with something.

The first challenge is where does the global exemplar configuration go? On the global configuration page, there is no ${instance}. There is only the descriptor. So what options are there?

#1 Create fields on the Descriptor to store these defaults.
#2 Store a reference to my global exemplar within the Descriptor singleton.

Of these two options, I like the second better. I don't have to duplicate fields in another class to simulate an exemplar. I actually just use an instance of the thing I'm making an example of - which makes sense. The drawback here is really more structural, Descriptors are usually nested. So here my nested singleton has a reference to a shared instance of its outer class. Its a little funny - but this is only an aesthetic issue.

Lets say I pick #2 for the sake of argument. I have a place to put my exemplar config now. The next challenge is that I need to update the correct object.

#1 Make the global.jelly use ${descriptor.exemplar.value } and the config.jelly use ${instance.value}.
#2 Use identical jelly's (jellies?) and differentiate in configure(StaplerRequest) which instance I should bindParameters() to based on the URI.
#3 Use identical jelly's and do something in the XML to set a reference to the instance to configure. If there is no ${instance} then use ${ descriptor.exemplar}

The first option is somewhat tedious for any non-trivial configuration. I have to write the same verbose jelly file twice, I have to change two places.

The second option allows me to use the same jelly file (in theory at least, I actually have to use copies since the stapler include tag here looks for included files under hudson/model/AbstractProject). I have to write less XML which is a giant plus, but it seems a little hacky to examine the URI and if starts with /hudson/job select one object other wise select another. When the URI changes, I'll have to update the code.

The third option is making me do some programming in XML which isn't great, but its only 1 if statement so maybe this isn't too bad.

Let's not pick one of these yet, the next challenge influences which one I can use right now.

Finally, the last challenge is that when you first create a Job, and that project configuration page appears for the first time, you'll have a bunch of fields on the screen - but you still won't get an ${instance} until the first time you submit this form.

This is probably the biggest challenge because when this page is drawn for the first time, what I really want to do is have the ability to initialize a new ExtensionPoint using the global exemplar. I *really* want to do this in Java, not jelly.

The only way to work around this at the moment is to put logic into jelly for each field which would be this psuedo-code:

  if(${instance} == null)
    ${descriptor.exemplar.field}
  else
    ${instance.field }

In this way, the page will be correctly rendered on its first display, and when the form is submitted we'd be able to bindParameters() to an instance of the ExensionPoint. On the next display, ${instance} won't be null and we'd be using it normally.

----

Ideas for things that could be improved, I'll list them in order of importance or difficulty.

* The first thing is I don't want to have this much logic in Jelly. This is really something that belongs in a factory since its very much tied to properly instantiating an ExensionPoint.

  One possibility would be to pass the StaplerRequest that requests the first load of a project config page into the Descriptor.newInstance() method to give ExentsionPoints a chance to initialize themselves (using the global exemplar if needed). The effect being that ${instance} in a config.jelly page would not be null when the page is first loaded and the user had a chance to initialize.
 
  Then you need a place for it. One place that seems logical is the Descriptor, getExemplarInstance() or something like that maybe?

* Descriptors must be singletons w/o exception,

In AbstractProject, addToList() and removeFromList() use == to add and remove extension points from a Project, so you actually will run into difficulty if anything but a singleton Descriptor is used. One way to break it is implement per-ExtensionPoint Descriptors. You'll find you'll never save your projects settings.

  This could be replaced with a Comparator that uses getClass() to perform the comparision test and have better results.

* There is an odd circular reference between ExtensionPoints and their factories, getDescriptor()

I've looked at this only because in some of what I expiremented with I really began to wonder why I'd getDescriptor() was implemented as it was. I tried to implement getDescriptor() in such a way that each ExtensionPoint instance created a new instance of its Descriptor. This, of course, fails because you wind up losing project setting you configure (due to the == test in AbstractProject I think).

I think making the change suggested about would allow for getDescriptor() to not have to be a singleton - but then I have to be careful about storing and global settings (Ant task style global settings) in static variables. What really bother me about getDescriptor() is that its easy to implement wrong, and its hard to confirm you implemented it right.

It doesn't look simple to remove getDescriptor(), it seems mainly to be used from within jelly in a few places, and I don't completely understand that code - so I can't make a rational suggestion about what would be better at this time. Maybe this is something you have though of too?

The benefit to there not being a getDescriptor() would be that you just create one, register it once when the plugin starts and you're done. There wouldn't been any fuzzy gotchas.

--

- Eric
Reply | Threaded
Open this post in threaded view
|

Re: Exemplar configuration thoughts

Kohsuke Kawaguchi-2

I wonder if the semantics you are suggested could be easily achieved
today by:

  1. create a project whose sole purpose is to be an exemplar
  2. create 'real' projects by copying it
  3. customize configurations of individual projects

There are obvious downsides to this, like once created the project will
not reflect any changes made in the exemplar, but I think it applies to
your suggestion below as well.

Or am I missing your point?

Eric Crahen wrote:

> I have a lot clearer picture of what I think might be needed to reduce the
> amount of per-project configuration required when many projects would use
> the same plugin with the same or almost identical configuration. I've been
> doing some experimentation and I've found a way to do what I'd like to do,
> but I think we can make some big improvements here. Some of it some of what
> you've said earlier. I thought it might be better all collected in an
> appropriately named post since I've come to this all in a roundabout way.
>
> First, I'll just briefly mention something about the global configuration.
> Terminology sometimes differes between people and is the source for a lot of
> confusion. The notion of global configuration we have right now is that its
> for settings that are orthogonal to the actual settings a Project would be
> interested in. For example,
>
> - The Ant task stores a global configuration in its Descriptor singleton.
> The values it stores are just the names and locations of the Ant
> installations available on the system. This configuration literally consists
> of names and paths. The Project configuration for the Ant task is stored in
> the instances of the Ant Builder member variables. The configuration for
> this component is not paths and names, its a reference to to the AntLocation
> to use.
>
> In this example, the global configuration is a definition, the project
> configuration values are a selection. Related, but orthogonal. This works
> very well for the sort of thing that Ant configures.
>
> The notion of global configuration I have been looking for is that of an
> exemplar global.
>
> - There is a global instance of an ExtensionPoint and configure it as an
> exemplar. The configuration is actually stored in an ExtensionPoint
> instance, and not in the Descriptor.
> - My global.jelly and config.jelly would look very similar, in fact they
> could be identical - the only difference is the instance of the
> ExtensionPoint being configured.
>
> This might seem similar because the word global keeps popping up (but thats
> what this clarification is about) The subtle difference here is that, the
> global configuration in this case is not orthogonal to the project
> configuration. It actually is the same thing.
>
> ---
>
> Now here are the challenges that I've been facing trying to come up with
> something.
>
> The first challenge is where does the global exemplar configuration go? On
> the global configuration page, there is no ${instance}. There is only the
> descriptor. So what options are there?
>
> #1 Create fields on the Descriptor to store these defaults.
> #2 Store a reference to my global exemplar within the Descriptor singleton.
>
> Of these two options, I like the second better. I don't have to duplicate
> fields in another class to simulate an exemplar. I actually just use an
> instance of the thing I'm making an example of - which makes sense. The
> drawback here is really more structural, Descriptors are usually nested. So
> here my nested singleton has a reference to a shared instance of its outer
> class. Its a little funny - but this is only an aesthetic issue.
>
> Lets say I pick #2 for the sake of argument. I have a place to put my
> exemplar config now. The next challenge is that I need to update the correct
> object.
>
> #1 Make the global.jelly use ${descriptor.exemplar.value} and the
> config.jelly use ${instance.value}.
> #2 Use identical jelly's (jellies?) and differentiate in
> configure(StaplerRequest) which instance I should bindParameters() to based
> on the URI.
> #3 Use identical jelly's and do something in the XML to set a reference to
> the instance to configure. If there is no ${instance} then use ${
> descriptor.exemplar}
>
> The first option is somewhat tedious for any non-trivial configuration. I
> have to write the same verbose jelly file twice, I have to change two
> places.
>
> The second option allows me to use the same jelly file (in theory at least,
> I actually have to use copies since the stapler include tag here looks for
> included files under hudson/model/AbstractProject). I have to write less XML
> which is a giant plus, but it seems a little hacky to examine the URI and if
> starts with /hudson/job select one object other wise select another. When
> the URI changes, I'll have to update the code.
>
> The third option is making me do some programming in XML which isn't great,
> but its only 1 if statement so maybe this isn't too bad.
>
> Let's not pick one of these yet, the next challenge influences which one I
> can use right now.
>
> Finally, the last challenge is that when you first create a Job, and that
> project configuration page appears for the first time, you'll have a bunch
> of fields on the screen - but you still won't get an ${instance} until the
> first time you submit this form.
>
> This is probably the biggest challenge because when this page is drawn for
> the first time, what I really want to do is have the ability to initialize a
> new ExtensionPoint using the global exemplar. I *really* want to do this in
> Java, not jelly.
>
> The only way to work around this at the moment is to put logic into jelly
> for each field which would be this psuedo-code:
>
>   if(${instance} == null)
>     ${descriptor.exemplar.field}
>   else
>     ${instance.field}
>
> In this way, the page will be correctly rendered on its first display, and
> when the form is submitted we'd be able to bindParameters() to an instance
> of the ExensionPoint. On the next display, ${instance} won't be null and
> we'd be using it normally.
>
> ----
>
> Ideas for things that could be improved, I'll list them in order of
> importance or difficulty.
>
> * The first thing is I don't want to have this much logic in Jelly. This is
> really something that belongs in a factory since its very much tied to
> properly instantiating an ExensionPoint.
>
>   One possibility would be to pass the StaplerRequest that requests the
> first load of a project config page into the Descriptor.newInstance() method
> to give ExentsionPoints a chance to initialize themselves (using the global
> exemplar if needed). The effect being that ${instance} in a
> config.jellypage would not be null when the page is first loaded and
> the user had a
> chance to initialize.
>
>   Then you need a place for it. One place that seems logical is the
> Descriptor, getExemplarInstance() or something like that maybe?
>
> * Descriptors must be singletons w/o exception,
>
> In AbstractProject, addToList() and removeFromList() use == to add and
> remove extension points from a Project, so you actually will run into
> difficulty if anything but a singleton Descriptor is used. One way to break
> it is implement per-ExtensionPoint Descriptors. You'll find you'll never
> save your projects settings.
>
>   This could be replaced with a Comparator that uses getClass() to perform
> the comparision test and have better results.
>
> * There is an odd circular reference between ExtensionPoints and their
> factories, getDescriptor()
>
> I've looked at this only because in some of what I expiremented with I
> really began to wonder why I'd getDescriptor() was implemented as it was. I
> tried to implement getDescriptor() in such a way that each ExtensionPoint
> instance created a new instance of its Descriptor. This, of course, fails
> because you wind up losing project setting you configure (due to the == test
> in AbstractProject I think).
>
> I think making the change suggested about would allow for getDescriptor() to
> not have to be a singleton - but then I have to be careful about storing and
> global settings (Ant task style global settings) in static variables. What
> really bother me about getDescriptor() is that its easy to implement wrong,
> and its hard to confirm you implemented it right.
>
> It doesn't look simple to remove getDescriptor(), it seems mainly to be used
> from within jelly in a few places, and I don't completely understand that
> code - so I can't make a rational suggestion about what would be better at
> this time. Maybe this is something you have though of too?
>
> The benefit to there not being a getDescriptor() would be that you just
> create one, register it once when the plugin starts and you're done. There
> wouldn't been any fuzzy gotchas.
>

--
Kohsuke Kawaguchi
Sun Microsystems                   [hidden email]

smime.p7s (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Exemplar configuration thoughts

Eric Crahen-3
That's an interesting work around. You did bring up a good point, though. It would be a handy thing to have an inhertence model that would allow all changes made to the exemplar to be reflected in the projects that did not override that property. I think that with what I described an ExtensionPoint author would pretty easily be able to write such an extension since he/she'd have a reference to the Exemplar and could delegate to it whenever configuration was absent in the project instance.

I could use the Project copy as a work around, but I feel like there might be a better solution for the longer term. (And there are still the issues about Descriptors and singleton-ness...)


On 1/29/07, Kohsuke Kawaguchi <[hidden email]> wrote:

I wonder if the semantics you are suggested could be easily achieved
today by:

  1. create a project whose sole purpose is to be an exemplar
  2. create 'real' projects by copying it
  3. customize configurations of individual projects

There are obvious downsides to this, like once created the project will
not reflect any changes made in the exemplar, but I think it applies to
your suggestion below as well.

Or am I missing your point?

Eric Crahen wrote:

> I have a lot clearer picture of what I think might be needed to reduce the
> amount of per-project configuration required when many projects would use
> the same plugin with the same or almost identical configuration. I've been
> doing some experimentation and I've found a way to do what I'd like to do,
> but I think we can make some big improvements here. Some of it some of what
> you've said earlier. I thought it might be better all collected in an
> appropriately named post since I've come to this all in a roundabout way.
>
> First, I'll just briefly mention something about the global configuration.
> Terminology sometimes differes between people and is the source for a lot of
> confusion. The notion of global configuration we have right now is that its
> for settings that are orthogonal to the actual settings a Project would be
> interested in. For example,
>
> - The Ant task stores a global configuration in its Descriptor singleton.
> The values it stores are just the names and locations of the Ant
> installations available on the system. This configuration literally consists
> of names and paths. The Project configuration for the Ant task is stored in
> the instances of the Ant Builder member variables. The configuration for
> this component is not paths and names, its a reference to to the AntLocation
> to use.
>
> In this example, the global configuration is a definition, the project
> configuration values are a selection. Related, but orthogonal. This works
> very well for the sort of thing that Ant configures.
>
> The notion of global configuration I have been looking for is that of an
> exemplar global.
>
> - There is a global instance of an ExtensionPoint and configure it as an
> exemplar. The configuration is actually stored in an ExtensionPoint
> instance, and not in the Descriptor.
> - My global.jelly and config.jelly would look very similar, in fact they
> could be identical - the only difference is the instance of the
> ExtensionPoint being configured.
>
> This might seem similar because the word global keeps popping up (but thats
> what this clarification is about) The subtle difference here is that, the
> global configuration in this case is not orthogonal to the project
> configuration. It actually is the same thing.
>
> ---

>
> Now here are the challenges that I've been facing trying to come up with
> something.
>
> The first challenge is where does the global exemplar configuration go? On
> the global configuration page, there is no ${instance}. There is only the
> descriptor. So what options are there?
>
> #1 Create fields on the Descriptor to store these defaults.
> #2 Store a reference to my global exemplar within the Descriptor singleton.
>
> Of these two options, I like the second better. I don't have to duplicate
> fields in another class to simulate an exemplar. I actually just use an
> instance of the thing I'm making an example of - which makes sense. The
> drawback here is really more structural, Descriptors are usually nested. So
> here my nested singleton has a reference to a shared instance of its outer
> class. Its a little funny - but this is only an aesthetic issue.
>
> Lets say I pick #2 for the sake of argument. I have a place to put my
> exemplar config now. The next challenge is that I need to update the correct
> object.
>
> #1 Make the global.jelly use ${descriptor.exemplar.value} and the
> config.jelly use ${instance.value }.
> #2 Use identical jelly's (jellies?) and differentiate in
> configure(StaplerRequest) which instance I should bindParameters() to based
> on the URI.
> #3 Use identical jelly's and do something in the XML to set a reference to
> the instance to configure. If there is no ${instance} then use ${
> descriptor.exemplar}
>
> The first option is somewhat tedious for any non-trivial configuration. I
> have to write the same verbose jelly file twice, I have to change two
> places.
>
> The second option allows me to use the same jelly file (in theory at least,
> I actually have to use copies since the stapler include tag here looks for
> included files under hudson/model/AbstractProject). I have to write less XML
> which is a giant plus, but it seems a little hacky to examine the URI and if
> starts with /hudson/job select one object other wise select another. When
> the URI changes, I'll have to update the code.
>
> The third option is making me do some programming in XML which isn't great,
> but its only 1 if statement so maybe this isn't too bad.
>
> Let's not pick one of these yet, the next challenge influences which one I
> can use right now.
>
> Finally, the last challenge is that when you first create a Job, and that
> project configuration page appears for the first time, you'll have a bunch
> of fields on the screen - but you still won't get an ${instance} until the
> first time you submit this form.
>
> This is probably the biggest challenge because when this page is drawn for
> the first time, what I really want to do is have the ability to initialize a
> new ExtensionPoint using the global exemplar. I *really* want to do this in

> Java, not jelly.
>
> The only way to work around this at the moment is to put logic into jelly
> for each field which would be this psuedo-code:
>
>   if(${instance} == null)
>     ${descriptor.exemplar.field}
>   else
>     ${instance.field}
>
> In this way, the page will be correctly rendered on its first display, and
> when the form is submitted we'd be able to bindParameters() to an instance
> of the ExensionPoint. On the next display, ${instance} won't be null and
> we'd be using it normally.
>
> ----
>
> Ideas for things that could be improved, I'll list them in order of
> importance or difficulty.
>
> * The first thing is I don't want to have this much logic in Jelly. This is
> really something that belongs in a factory since its very much tied to

> properly instantiating an ExensionPoint.
>
>   One possibility would be to pass the StaplerRequest that requests the
> first load of a project config page into the Descriptor.newInstance() method
> to give ExentsionPoints a chance to initialize themselves (using the global
> exemplar if needed). The effect being that ${instance} in a
> config.jellypage would not be null when the page is first loaded and
> the user had a
> chance to initialize.
>
>   Then you need a place for it. One place that seems logical is the
> Descriptor, getExemplarInstance() or something like that maybe?
>
> * Descriptors must be singletons w/o exception,
>
> In AbstractProject, addToList() and removeFromList() use == to add and
> remove extension points from a Project, so you actually will run into
> difficulty if anything but a singleton Descriptor is used. One way to break

> it is implement per-ExtensionPoint Descriptors. You'll find you'll never
> save your projects settings.
>
>   This could be replaced with a Comparator that uses getClass() to perform
> the comparision test and have better results.
>
> * There is an odd circular reference between ExtensionPoints and their
> factories, getDescriptor()
>
> I've looked at this only because in some of what I expiremented with I
> really began to wonder why I'd getDescriptor() was implemented as it was. I
> tried to implement getDescriptor() in such a way that each ExtensionPoint
> instance created a new instance of its Descriptor. This, of course, fails
> because you wind up losing project setting you configure (due to the == test
> in AbstractProject I think).
>
> I think making the change suggested about would allow for getDescriptor() to
> not have to be a singleton - but then I have to be careful about storing and
> global settings (Ant task style global settings) in static variables. What
> really bother me about getDescriptor() is that its easy to implement wrong,
> and its hard to confirm you implemented it right.
>
> It doesn't look simple to remove getDescriptor(), it seems mainly to be used
> from within jelly in a few places, and I don't completely understand that
> code - so I can't make a rational suggestion about what would be better at
> this time. Maybe this is something you have though of too?
>
> The benefit to there not being a getDescriptor() would be that you just
> create one, register it once when the plugin starts and you're done. There
> wouldn't been any fuzzy gotchas.
>


--
Kohsuke Kawaguchi
Sun Microsystems                   [hidden email]




--

- Eric
Reply | Threaded
Open this post in threaded view
|

Re: Exemplar configuration thoughts

Kohsuke Kawaguchi-2
Eric Crahen wrote:
> That's an interesting work around. You did bring up a good point, though. It
> would be a handy thing to have an inhertence model that would allow all
> changes made to the exemplar to be reflected in the projects that did not
> override that property. I think that with what I described an ExtensionPoint
> author would pretty easily be able to write such an extension since he/she'd
> have a reference to the Exemplar and could delegate to it whenever
> configuration was absent in the project instance.

Inheritance seems to be a fairly popular notion among other tools but
giving it a decent GUI is rather hard. Somehow you have to differentiate
overridden values and default values (and it has to survive during the
form submission round-trip), and you have to let people revert the setting.

For systems that don't provide GUI, it's pretty easy, but for us, well...



> I could use the Project copy as a work around, but I feel like there might
> be a better solution for the longer term. (And there are still the issues
> about Descriptors and singleton-ness...)

I read your e-mail but it's not quite clear to me what problems you had
with Descriptors.

Describables/Descriptors are really like Objects/Classes, so I find
singleton semantics appropriate. If "it's easy to implement wrong,
and it's hard to confirm you implemented it right" as you say, I'd like
to fix it, though. Maybe I need to expand on Describable.getDescriptor()
javadoc?

--
Kohsuke Kawaguchi
Sun Microsystems                   [hidden email]

smime.p7s (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Exemplar configuration thoughts

Kohsuke Kawaguchi-2
Kohsuke Kawaguchi wrote:
> Inheritance seems to be a fairly popular notion among other tools but
> giving it a decent GUI is rather hard. Somehow you have to differentiate
> overridden values and default values (and it has to survive during the
> form submission round-trip), and you have to let people revert the setting.
>
> For systems that don't provide GUI, it's pretty easy, but for us, well...

I guess another thing is that so far it sounds like you don't really
need the inheritance. For example, in issue #257 you say that your group
enforces the directory layout where you put artifacts in build/, so you
don't really need to let people override it.

I agree that it would be nice to do something in this area, but I'm
trying to avoid doing inheritance, I guess...



--
Kohsuke Kawaguchi
Sun Microsystems                   [hidden email]

smime.p7s (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Exemplar configuration thoughts

Eric Crahen-3
In reply to this post by Kohsuke Kawaguchi-2
On 1/29/07, Kohsuke Kawaguchi <[hidden email]> wrote:
I read your e-mail but it's not quite clear to me what problems you had
with Descriptors.

Describables/Descriptors are really like Objects/Classes, so I find
singleton semantics appropriate. If "it's easy to implement wrong,
and it's hard to confirm you implemented it right" as you say, I'd like
to fix it, though. Maybe I need to expand on Describable.getDescriptor()
javadoc?

I think a clearer explaination in the javadoc would help. I only figured out that it had to be a singleton after a lot of trial and error.
Other than that, I think it was just the circular dependency between product and factory. I couldn't find anywhere that the code really needed to ask a product (ExtensionPoint) what its factory (Descriptor) was, sans the jelly. That made me start thinking about weather there was a real value there since it didn't seem to be exploited. Oh, the other thing that made it confusing with out documentation was how the classes were nested. The Descriptors being nested static classes, within the product ExtensionPoint they create. Maybe its just me, but the factory class being nested within the thing it creates just made it a little harder to wrap my head around, though I'm sure its not a new idiom.

I do see it how it works better when I think of it in the Object/Class analogy though. That would be a good way to explain it in the docs.

The main difference I'm talking about when I say not so easy to implement right is that the end user really needs to understand that Descriptors are singletons and implement them as such. You can't enforce that or communicate that with a function signature. The language doesn't allow people the option to return a class other than what Object.getClass() dictates, that makes it really hard for a user to get wrong. I guess what I'm saying is from something like Object/Class there is enforcement there and the only way you can implement it is the "right way", where as with ExtensionPoint/Descriptor you have more rope to hang yourself with.

Now if you took getDescriptor() away, you suddenly don't have the problem of requiring a singleton. Any Descriptor could make the ExtensionPoint. You only need to add one to the Hudson instance, and I think that would be it. There's not a whole a lot of getDescriptor() that seemed to be needed, so this seemed simpler to me since the circular dependency is gone, and the singleton requirement is gone.

This was something I was starting to think about and I think I would be inclined to reduce the coupling between those two classes. That's just me though. Ultimately its your project, (which is great so far btw), so its up to you.


--

- Eric
Reply | Threaded
Open this post in threaded view
|

Re: Exemplar configuration thoughts

Eric Crahen-3
In reply to this post by Kohsuke Kawaguchi-2
I was thinking I would use inheritence to set other options. The plugins I was working on make all the various options for stuff like FindBugs clickable - so theres a bunch there you could configure and inherit.

I can do the work around for now, I wouldn't rush into an inheritence design. It definently sounds like something that needs a lot of thought and your solution of copying the projects would be fine for me for a while.


On 1/29/07, Kohsuke Kawaguchi <[hidden email]> wrote:
Kohsuke Kawaguchi wrote:
> Inheritance seems to be a fairly popular notion among other tools but
> giving it a decent GUI is rather hard. Somehow you have to differentiate
> overridden values and default values (and it has to survive during the
> form submission round-trip), and you have to let people revert the setting.
>
> For systems that don't provide GUI, it's pretty easy, but for us, well...

I guess another thing is that so far it sounds like you don't really
need the inheritance. For example, in issue #257 you say that your group
enforces the directory layout where you put artifacts in build/, so you
don't really need to let people override it.

I agree that it would be nice to do something in this area, but I'm
trying to avoid doing inheritance, I guess...



--
Kohsuke Kawaguchi
Sun Microsystems                   [hidden email]





--

- Eric
Reply | Threaded
Open this post in threaded view
|

Re: Exemplar configuration thoughts

Kohsuke Kawaguchi-2
In reply to this post by Eric Crahen-3
Eric Crahen wrote:

> On 1/29/07, Kohsuke Kawaguchi <[hidden email]> wrote:
>>
>> I read your e-mail but it's not quite clear to me what problems you had
>> with Descriptors.
>>
>> Describables/Descriptors are really like Objects/Classes, so I find
>> singleton semantics appropriate. If "it's easy to implement wrong,
>> and it's hard to confirm you implemented it right" as you say, I'd like
>> to fix it, though. Maybe I need to expand on Describable.getDescriptor()
>> javadoc?
>
>
> I think a clearer explaination in the javadoc would help. I only figured out
> that it had to be a singleton after a lot of trial and error.
> Other than that, I think it was just the circular dependency between product
> and factory. I couldn't find anywhere that the code really needed to ask a
> product (ExtensionPoint) what its factory (Descriptor) was, sans the jelly.
> That made me start thinking about weather there was a real value there since
> it didn't seem to be exploited. Oh, the other thing that made it confusing
> with out documentation was how the classes were nested. The Descriptors
> being nested static classes, within the product ExtensionPoint they create.
> Maybe its just me, but the factory class being nested within the thing it
> creates just made it a little harder to wrap my head around, though I'm sure
> its not a new idiom.
I see. I guess you are right that having descriptor as an inner class is
unnecessarily confusing. I did it so just because that keeps the number
of classes in my package explorer smaller.


> The main difference I'm talking about when I say not so easy to implement
> right is that the end user really needs to understand that Descriptors are
> singletons and implement them as such. You can't enforce that or communicate
> that with a function signature. The language doesn't allow people the option
> to return a class other than what Object.getClass() dictates, that makes it
> really hard for a user to get wrong. I guess what I'm saying is from
> something like Object/Class there is enforcement there and the only way you
> can implement it is the "right way", where as with ExtensionPoint/Descriptor
> you have more rope to hang yourself with.

True.

> Now if you took getDescriptor() away, you suddenly don't have the problem of
> requiring a singleton. Any Descriptor could make the ExtensionPoint. You
> only need to add one to the Hudson instance, and I think that would be it.
> There's not a whole a lot of getDescriptor() that seemed to be needed, so
> this seemed simpler to me since the circular dependency is gone, and the
> singleton requirement is gone.

I guess this is where I need to understand you better. For example,
today I need descriptors for listing up the kind of Jobs that are
installed. If I remove getDescriptor(), how do I do that? and how do I
create a new Job instance?

Or once loaded, the global configuration (like where Ant is installed)
is stored in Ant's Descriptor object. If you get rid of that, where do
you keep it, in a way easy to persist?


> This was something I was starting to think about and I think I would be
> inclined to reduce the coupling between those two classes. That's just me
> though. Ultimately its your project, (which is great so far btw), so its up
> to you.

I really appreciate all your comments.

I'd like to get more contributions, so I don't want to cut a discussion
by declaring "I'll do it this way because this is mine!"


--
Kohsuke Kawaguchi
Sun Microsystems                   [hidden email]

smime.p7s (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Exemplar configuration thoughts

Eric Crahen-3

On 1/29/07, Kohsuke Kawaguchi <[hidden email]> wrote:

> Now if you took getDescriptor() away, you suddenly don't have the problem of
> requiring a singleton. Any Descriptor could make the ExtensionPoint. You
> only need to add one to the Hudson instance, and I think that would be it.
> There's not a whole a lot of getDescriptor() that seemed to be needed, so
> this seemed simpler to me since the circular dependency is gone, and the
> singleton requirement is gone.

I guess this is where I need to understand you better. For example,
today I need descriptors for listing up the kind of Jobs that are
installed.
 
I'm really not that solid on my jelly skills yet, so some of what happens between the XML and the Java is a bit of a mystery but I'll try and show you what I was thinking of.

To list up the various kinds of Jobs,

// Not-the-current-APIs, just making what I'm doing clear

// Create a Hudson instance
Hudson h = new Hudson();

// Give it the JobDescriptors
h.addJobDescriptor ( new JPRTJobDescriptor() );
h.addJobDescriptor( new ExternalJobDescriptor() );
h.addJobDescriptor( new HudsonJobDescriptor() );

// When the jelly page that needs the job specific descriptors is created,
// whatever it is that gets things into jelly could hand it this,

Descriptor[] jobDescriptors = h.getJobDescriptors();

I don't need to have an existing Job to get a list of all the Descriptors that I know about.

If I remove getDescriptor(), how do I do that? and how do I
create a new Job instance?

The descriptors could still be used to instantiate the new Job instance.
I don't need to have an existing Job to make a brand new one.

// I'm not sure how I get the StaplerRequest in exact detail, but someone
// clicks the New Job, and the selected descriptor from the list that we
// gave the jelly page earlier is used

StaplerRequest req = ...;
Descriptor jobDescriptor = ...;
jobDescriptor.newInstance(req);

The Job instances themselves could be used to copy an existing Job using clone().
I do need a Job to create a copy but I don't need the Descriptor.

Or once loaded, the global configuration (like where Ant is installed)
is stored in Ant's Descriptor object. If you get rid of that, where do
you keep it, in a way easy to persist?

I can still share a state between these objects without a getDescriptor()

// Put the AntBuilder into Hudsone
h.addBuilderDescriptor( new HudsonAntDescriptor() );

class Ant extends ... {

  private final AntInstallation[] installations;

  Ant(AntInstallation[] installations) {
    this.installations = installations;
  }

  // ...

}

class AntDescriptor extends ... {
  
  private AntInstallation[] installations = ...;
 
  // ...

  public Builder newInstance(StaplerRequest req) {
    return new Ant(installations);
  }

  // ...

}

I'm not totally familar with XStream, so I'm not sure if this would store a duplicate - so maybe one of these fields needs to be marked transient. This is more of an XStream detail though and less what I'm trying to illustrate. If I need the semantics to be that the list of AntInstallations is shared and not copied, then I can share a j.u.l.c.CopyOnWriteArrayList.

When the global config page is displayed, we'd collect from the Hudson instance the arrays of the various Descriptor types and stick them into the jelly page.

The addExtensionPoint methods I was using on the Hudson instance to register the Descriptors actually might add to a HashSet for each type of Descriptor, eliminating duplicate Descriptors.


> This was something I was starting to think about and I think I would be
> inclined to reduce the coupling between those two classes. That's just me
> though. Ultimately its your project, (which is great so far btw), so its up
> to you.

I really appreciate all your comments.

I'd like to get more contributions, so I don't want to cut a discussion
by declaring "I'll do it this way because this is mine!"


--
Kohsuke Kawaguchi
Sun Microsystems                   [hidden email]





--

- Eric