Re: svn commit: r1325196 - in /tomcat/maven-plugin/trunk: (...)

2012-04-12 Thread Olivier Lamy 
Just fixed that
Thanks for the review!


2012/4/12 Konstantin Kolinko :
> 2012/4/12  :
>> Author: olamy
>> Date: Thu Apr 12 10:32:12 2012
>> New Revision: 1325196
>>
>> URL: http://svn.apache.org/viewvc?rev=1325196&view=rev
>> Log:
>> fix field name typo
>>
>> Modified:
>>    
>> tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java
>>    
>> tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java
>>
>> Modified: 
>> tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java
>> URL: 
>> http://svn.apache.org/viewvc/tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java?rev=1325196&r1=1325195&r2=1325196&view=diff
>> ==
>> --- 
>> tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java
>>  (original)
>> +++ 
>> tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java
>>  Thu Apr 12 10:32:12 2012
>> @@ -343,7 +343,7 @@ public abstract class AbstractRunMojo
>>      */
>>     private ClassRealm tomcatRealm;
>>
>> -    private ClassLoader originalClassLoaser;
>> +    private ClassLoader originalClassLoader;
>>
>>     /**
>>      * The static context
>> @@ -383,13 +383,13 @@ public abstract class AbstractRunMojo
>>             getLog().info( messagesProvider.getMessage( 
>> "AbstractRunMojo.nonWar" ) );
>>             return;
>>         }
>> -        originalClassLoaser = 
>> Thread.currentThread().getContextClassLoader();
>> +        originalClassLoader = 
>> Thread.currentThread().getContextClassLoader();
>>         try
>>         {
>>
>>             if ( useSeparateTomcatClassLoader )
>>             {
>> -                originalClassLoaser = 
>> Thread.currentThread().getContextClassLoader();
>> +                originalClassLoader = 
>> Thread.currentThread().getContextClassLoader();
>>             }
>>             getLog().info( messagesProvider.getMessage( 
>> "AbstractRunMojo.runningWar", getWebappUrl() ) );
>>
>> @@ -413,7 +413,7 @@ public abstract class AbstractRunMojo
>>         {
>>             if ( useSeparateTomcatClassLoader )
>>             {
>> -                Thread.currentThread().setContextClassLoader( 
>> originalClassLoaser );
>> +                Thread.currentThread().setContextClassLoader( 
>> originalClassLoader );
>>             }
>>         }
>>     }
>>
>> Modified: 
>> tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java
>> URL: 
>> http://svn.apache.org/viewvc/tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java?rev=1325196&r1=1325195&r2=1325196&view=diff
>> ==
>> --- 
>> tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java
>>  (original)
>> +++ 
>> tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java
>>  Thu Apr 12 10:32:12 2012
>> @@ -363,7 +363,7 @@ public abstract class AbstractRunMojo
>>      */
>>     private ClassRealm tomcatRealm;
>>
>> -    private ClassLoader originalClassLoaser;
>> +    private ClassLoader originalClassLoader;
>>
>>     // --
>>     // Mojo Implementation
>> @@ -388,7 +388,7 @@ public abstract class AbstractRunMojo
>>         }
>>         if ( useSeparateTomcatClassLoader )
>>         {
>> -            originalClassLoaser = 
>> Thread.currentThread().getContextClassLoader();
>> +            originalClassLoader = 
>> Thread.currentThread().getContextClassLoader();
>>         }
>>         try
>>         {
>> @@ -418,7 +418,7 @@ public abstract class AbstractRunMojo
>>         {
>>             if ( useSeparateTomcatClassLoader )
>>             {
>> -                Thread.currentThread().setContextClassLoader( 
>> originalClassLoaser );
>> +                Thread.currentThread().setContextClassLoader( 
>> originalClassLoader );
>>             }
>>         }
>>     }
>
> 1. Do you need to keep "originalClassLoader" as a class field?
>
> As far as I see its use is scoped to the execute() method, so that it
> can be a local variable there.
>
>
> 2. Tomcat 6 and Tomcat 7 parts of this commit do not match each other.
>  I mean - see where the value to originalClassLoader is assigned.
>
> There is "if" missing in Tomcat 6 and the second invocation inside of
> "try{}" was not removed.
>
> Best regards,
> Konstantin Kolinko
>
> ---

Re: svn commit: r1325196 - in /tomcat/maven-plugin/trunk: (...)

2012-04-12 Thread Konstantin Kolinko 
2012/4/12  :
> Author: olamy
> Date: Thu Apr 12 10:32:12 2012
> New Revision: 1325196
>
> URL: http://svn.apache.org/viewvc?rev=1325196&view=rev
> Log:
> fix field name typo
>
> Modified:
>    
> tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java
>    
> tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java
>
> Modified: 
> tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java
> URL: 
> http://svn.apache.org/viewvc/tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java?rev=1325196&r1=1325195&r2=1325196&view=diff
> ==
> --- 
> tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java
>  (original)
> +++ 
> tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java
>  Thu Apr 12 10:32:12 2012
> @@ -343,7 +343,7 @@ public abstract class AbstractRunMojo
>      */
>     private ClassRealm tomcatRealm;
>
> -    private ClassLoader originalClassLoaser;
> +    private ClassLoader originalClassLoader;
>
>     /**
>      * The static context
> @@ -383,13 +383,13 @@ public abstract class AbstractRunMojo
>             getLog().info( messagesProvider.getMessage( 
> "AbstractRunMojo.nonWar" ) );
>             return;
>         }
> -        originalClassLoaser = Thread.currentThread().getContextClassLoader();
> +        originalClassLoader = Thread.currentThread().getContextClassLoader();
>         try
>         {
>
>             if ( useSeparateTomcatClassLoader )
>             {
> -                originalClassLoaser = 
> Thread.currentThread().getContextClassLoader();
> +                originalClassLoader = 
> Thread.currentThread().getContextClassLoader();
>             }
>             getLog().info( messagesProvider.getMessage( 
> "AbstractRunMojo.runningWar", getWebappUrl() ) );
>
> @@ -413,7 +413,7 @@ public abstract class AbstractRunMojo
>         {
>             if ( useSeparateTomcatClassLoader )
>             {
> -                Thread.currentThread().setContextClassLoader( 
> originalClassLoaser );
> +                Thread.currentThread().setContextClassLoader( 
> originalClassLoader );
>             }
>         }
>     }
>
> Modified: 
> tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java
> URL: 
> http://svn.apache.org/viewvc/tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java?rev=1325196&r1=1325195&r2=1325196&view=diff
> ==
> --- 
> tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java
>  (original)
> +++ 
> tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java
>  Thu Apr 12 10:32:12 2012
> @@ -363,7 +363,7 @@ public abstract class AbstractRunMojo
>      */
>     private ClassRealm tomcatRealm;
>
> -    private ClassLoader originalClassLoaser;
> +    private ClassLoader originalClassLoader;
>
>     // --
>     // Mojo Implementation
> @@ -388,7 +388,7 @@ public abstract class AbstractRunMojo
>         }
>         if ( useSeparateTomcatClassLoader )
>         {
> -            originalClassLoaser = 
> Thread.currentThread().getContextClassLoader();
> +            originalClassLoader = 
> Thread.currentThread().getContextClassLoader();
>         }
>         try
>         {
> @@ -418,7 +418,7 @@ public abstract class AbstractRunMojo
>         {
>             if ( useSeparateTomcatClassLoader )
>             {
> -                Thread.currentThread().setContextClassLoader( 
> originalClassLoaser );
> +                Thread.currentThread().setContextClassLoader( 
> originalClassLoader );
>             }
>         }
>     }

1. Do you need to keep "originalClassLoader" as a class field?

As far as I see its use is scoped to the execute() method, so that it
can be a local variable there.


2. Tomcat 6 and Tomcat 7 parts of this commit do not match each other.
 I mean - see where the value to originalClassLoader is assigned.

There is "if" missing in Tomcat 6 and the second invocation inside of
"try{}" was not removed.

Best regards,
Konstantin Kolinko

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org