+1

Cheers,
Guillaume Nodet

David Jencks wrote:

I've uploaded

http://issues.apache.org/jira/secure/attachment/12335128/geronimo- deployment-plugin-RTC-VOTE.2.patch

incorporating most of your comments, see below.

Thanks for the review!

david jencks

On Jun 6, 2006, at 11:57 PM, Jacek Laskowski wrote:

On 6/6/06, David Jencks <[EMAIL PROTECTED]> wrote:

Prasad has been working for a long time on an m2 deployment plugin.
I've cleaned up his latest patch a little bit and think its ready to
commit.

Please review http://issues.apache.org/jira/secure/attachment/
12335116/geronimo-deployment-plugin-RTC-VOTE.patch


I have taken a look at the patch and the comments are as follows
(they're rather style-centric nor technical, but still valid I hope).
No testing was performed.

1/ I was scared to read: "May not have been tested." in one of the classes


You want me to lie :-) ? I don't think anyone has ever used the in-vm startServer command in the m1 plugin, so I doubt prasad has tested this one either. I still think its worth including as a starting point in case some one wants to try it out.


2/ Copyright 2004-2006 - shouldn't it be Copyright 2006 only?


These are basically copies + modifications of the m1 deployment plugin, copyright 2004. I don't know if any changes happened in 2005, but 2004 and 2006 are definitely needed.


3/ The javadoc of classes should be consistent. It means that it should read:

  /**
   * @goal undeploy (if appropriate)
   *
   * @version $Rev$ $Date$
   */

whereas some contain

  @version $Revision$ $Date$

or no version at all.


They should all have the @version, but only mojos should have the @goal in order to not confuse maven. I've fixed the @version tags as well as I can find them.


4/ Geronimo :: Maven Deployment Plugin using m2 -> Geronimo :: Maven 2
Deployment Plugin or alternatively Geronimo :: Maven Deployment Plugin
for Maven 2, but I'd prefer the former.


fixed


5/ geronimo-deployment-plugin/pom.xml has no ASF license header.

fixed. I think its better to include the asf license in the source even if maven removes it during processing.



So, unless it's corrected I'm -1. If you're swampped with your other
work, I can take care of it and propose the patch corrected again.

Here's my +1.


It doesn't count, though ;-)


Why not? Because I edited prasad's patch for formatting and removed unused cruft? Ken's directive requires 3 +1 votes from committers other than the proposer (who apparently does not need to be a committer): although the document he appears to have adapted states only PMC member votes count, that is not in his directive. Since he hasn't responded to requests for clarification I think we have to take him at his word.

thanks
david jencks




david jencks


Jacek

--
Jacek Laskowski
http://www.laskowski.net.pl




Reply via email to