Maven 1.x Announcement Plugin

Patch with the proposed enhancement. Includes:

• plugin.jelly
• xdocs/properties.xml
• xdocs/changes.xml
• project.xml

PS: I didn't provide any unit test case because I think it's not possible for code developed in Jelly. Am I'm right?

Hi Felipe,

Thanks for the patch. Here are some comments/questions:

1/ I'm interested to know why you need to provide your own stylesheet. Is there anything missing in the existing that we could add? How are you using the new properties you've set?

2/ I don't think there's a need for the stylesheet "Type". Simply define the location and you're done.

3/ Yes, there should be unit tests. We need to add tests. Have a look at some other plugins like checkstyle, changes, etc. Check in src/plugin-test. Tests are run by calling "maven plugin:test".

Thanks!
-Vincent

> Thanks for the patch. Here are some comments/questions:

Thank you for the quick reply - patches/bug reports to popular projects like Maven usually take longer to be answered

> 1/ I'm interested to know why you need to provide your own stylesheet.

We use a different format for our email announcement (by the way, we're also working in a patch that sends the announcement by mail - I will submit it under MPANNOUNCEMENT-9 once it's ready). Besides being in a different language (Portuguese), we need more info, like:

• SCM tag used for the release
• id of the last snapshot
• more URLS (documentation, downloads, etc...)

Of course, we could just change the content of anouncement.jcl, but that is not an elegant solution - what if I need 2 different templates, for instance? (In fact, we will face that situation in the future, as some of our components will be distributed internally and externally).

> Is there anything missing in the existing that we could add?

Yes, we have those specific needs mentioned before. And other organizations might have different ones, so we better provide a configurable template than trying to include everything ourselves.

> How are you using the new properties you've set?

Right now, we are setting the value property globally (i.e., in the multiproject root) and using the default value of 'resource' (the reason for this option is that we are using our customized version of maven, so the new announcement file get automatically installed at $MAVEN_REPO)

> 2/ I don't think there's a need for the stylesheet "Type". Simply
> define the location and you're done.

I think the type make it more flexible. For instance, in our case we are using the resource, as we are installing a customized version of the plugin. Most of the time, though, I think people could use 'uri' for a global/generic announcement (defined at multiproject level, for instance) and 'project' for a project specific one.

> 3/ Yes, there should be unit tests. We need to add tests. Have a
> look at some other plugins like checkstyle, changes, etc. Check in
> src/plugin-test. Tests are run by calling "maven plugin:test".

Ok, I will take a look on them and write the tests later (once we decided how the new feature will be implemented, for instance, having the type property or not).

Regards,

Felipe

> We use a different format for our email announcement (by the way,
> we're also working in a patch that sends the announcement by mail -
> I will submit it under MPANNOUNCEMENT-9 once it's ready).

cool

> Besides being in a different language (Portuguese), we need more
> info, like:
>
> - SCM tag used for the release

What for?

> - id of the last snapshot

What for?

> - more URLS (documentation, downloads, etc...)

Could you show a sample announcement using your own stylesheet. Maybe we could integrate those changes in the default one if it's generic enough (and useful)?

> I think the type make it more flexible. For instance, in our case
> we are using the resource, as we are installing a customized
> version of the plugin. Most of the time, though, I think people
> could use 'uri' for a global/generic announcement (defined at
> multiproject level, for instance) and 'project' for a project
> specific one.

I don't think you understand what I mean. I'm saying there's no need for a type because the full stylesheet path can be specified in the stylesheet property.

Thanks! It looks good. I'm happy to apply your patch without the type property (or you need to convince me about the type! ).

> - SCM tag used for the release
> - id of the last snapshot
> What for?

These are part of our internal release management process. We need to inform these data everytime a new release is released.

> - more URLS (documentation, downloads, etc...)

> Could you show a sample announcement using your own stylesheet.
> Maybe we could integrate those changes in the default one if it's
> generic enough (and useful)?

Ok, I will include that in the next patch version.

> I don't think you understand what I mean. I'm saying there's no need
> for a type because the full stylesheet path can be specified in the
> stylesheet property.

The only problem I see with a full path is that it would require knowing the physical location of the file or some sort of EL hack to obtain it (ex: path=${basedir}/etc/myAnnouncements.jsl). I agree that adding a type increases the complexity, but I still think that option is usefull. What if we keep the Type, but set it as 'uri' by default?

> Thanks! It looks good. I'm happy to apply your patch without the
> type property (or you need to convince me about the type! ).

Hopefully my last paragraph changed your mind

> The only problem I see with a full path is that it would require
> knowing the physical location of the file or some sort of EL hack
> to obtain it (ex: path=${basedir}/etc/myAnnouncements.jsl). I
> agree that adding a type increases the complexity, but I still
> think that option is usefull. What if we keep the Type, but set it
> as 'uri' by default?

How is the following a problem?

Custom (user overriden):
path = ${basedir}/conf/myannouncement.jsl

Default:
path = ${plugin.resources}/announcement.jsl

Using a type property is much worse IMO as it will only permit the locations that are hard-coded in the script which is limitating, more complex and simply unnecessary.

> How is the following a problem?

> Custom (user overriden):
> path = ${basedir}/conf/myannouncement.jsl

> Default:
> path = ${plugin.resources}/announcement.jsl

That's simple, I agree. My only concern is regarding multi-project: if I have many sub-projects with share the same announcement, I would like to define that property at the root level only(i.e., from where multi-project is called). Would ${basedir} in this case solve to the root's dir or to each sub-project being 'reacted'? If not, is there any other option or would I need to set a physical path? (Sorry for the RTFMish aspect of the question, but I'm not that familiar with multiproject yet).

Vince,

Looks like nobody in the list was interested in this discussion

So, here is a new patch using your simpler solution.

Regards,

Felipe

Applied (with minor modifications). Thanks.