Hi Brett,

On Sat, Apr 23, 2011 at 7:54 AM, Brett Porter <[email protected]> wrote:

>
> On 23/04/2011, at 1:53 AM, [email protected] wrote:
>
> > Modified:
> incubator/npanday/trunk/components/dotnet-core/src/main/resources/META-INF/npanday/registry-config.xml
> > URL:
> http://svn.apache.org/viewvc/incubator/npanday/trunk/components/dotnet-core/src/main/resources/META-INF/npanday/registry-config.xml?rev=1095951&r1=1095950&r2=1095951&view=diff
> >
> ==============================================================================
> > ---
> incubator/npanday/trunk/components/dotnet-core/src/main/resources/META-INF/npanday/registry-config.xml
> (original)
> > +++
> incubator/npanday/trunk/components/dotnet-core/src/main/resources/META-INF/npanday/registry-config.xml
> Fri Apr 22 15:53:58 2011
> > @@ -22,7 +22,7 @@ under the License.
> >     <repository>
> >       <repository-name>npanday-settings</repository-name>
> >
> <repository-class>npanday.vendor.impl.SettingsRepository</repository-class>
> > -
>  <repository-config>${user.home}/.m2/npanday-settings.xml</repository-config>
> > +      <repository-config>${npanday.settings}</repository-config>
> >       <init-param>
> >         <param-name>optional</param-name>
> >         <param-value>true</param-value>
>
> Will this work if the user changes it on the mojo configuration or
> settings.xml change and doesn't use the -Dnpanday.settings argument?
>


Yes, it still works even if you configure it in the mojo.


>
> >
> > Modified:
> incubator/npanday/trunk/plugins/maven-aspx-plugin/src/main/java/npanday/plugin/aspx/AspxCompilerMojo.java
> > URL:
> http://svn.apache.org/viewvc/incubator/npanday/trunk/plugins/maven-aspx-plugin/src/main/java/npanday/plugin/aspx/AspxCompilerMojo.java?rev=1095951&r1=1095950&r2=1095951&view=diff
> >
> ==============================================================================
> > ---
> incubator/npanday/trunk/plugins/maven-aspx-plugin/src/main/java/npanday/plugin/aspx/AspxCompilerMojo.java
> (original)
> > +++
> incubator/npanday/trunk/plugins/maven-aspx-plugin/src/main/java/npanday/plugin/aspx/AspxCompilerMojo.java
> Fri Apr 22 15:53:58 2011
> > @@ -22,6 +22,7 @@ package npanday.plugin.aspx;
> > import java.io.File;
> > import java.io.IOException;
> > import java.util.ArrayList;
> > +import java.util.Hashtable;
> > import java.util.List;
> >
> > import npanday.ArtifactType;
> > @@ -30,7 +31,10 @@ import npanday.executable.ExecutionExcep
> > import npanday.executable.compiler.CompilerConfig;
> > import npanday.executable.compiler.CompilerExecutable;
> > import npanday.executable.compiler.CompilerRequirement;
> > +import npanday.registry.RepositoryRegistry;
> > +import npanday.registry.impl.StandardRepositoryLoader;
> > import npanday.vendor.VendorFactory;
> > +import npanday.vendor.impl.SettingsRepository;
> > import org.apache.maven.plugin.AbstractMojo;
> > import org.apache.maven.plugin.MojoExecutionException;
> > import org.apache.maven.project.MavenProject;
> > @@ -52,6 +56,11 @@ public class AspxCompilerMojo
> >     private static final String DEFAULT_EXCLUDES = "obj/**, target/**,
> **/*.pdb, **/*.csproj, **/*.vbproj, **/*.suo, **/*.user,pom.xml,
> **/*.sln,build.log,PrecompiledApp.config,csproj.user,Properties/**,**.releaseBackup,^-?(?:\\d+|\\d{1,3}(?:,\\d{3})+)(?:\\.\\d+)?$/**";
> >
> >     /**
> > +     * @parameter expression ="${npanday.settings}"
> > +     */
> > +    private String settingsPath;
> > +
>
> Above, npanday.settings refers to a file. Here, it refers to a path. I
> think you should change this to private File settingsFile and adjust
> accordingly.
>
>

The variable in registry-config.xml is not really needed because the value
will be based on the ${npanday.settings} configured in the mojo and if
there's no configuration the default-value (which is ${user.home}/.m2) will
be used.



> Also, you can add default-value="${user.home}/.m2/npanday-settings.xml" to
> avoid the extra logic below.
>

> > +        getNPandaySettingsPath();
>
> This is a confusing name - the method doesn't return anything (it doesn't
> "get"), and mostly works on things other than the path. How about
> "populateSettingsRepository" ?
>
> > +    protected void getNPandaySettingsPath()
> > +    {
> > +        if ( settingsPath == null )
> > +        {
> > +            settingsPath = System.getProperty( "user.home" ) + "/.m2";
> > +        }
>
> This can be removed if the above default is set.
> >
>


I've revised the method name to populateSettingsRepository when populating
the value for ${npanday.settings} and also added a default value for
${npanday.settings} so additional logic was removed



> > Modified:
> incubator/npanday/trunk/plugins/maven-compile-plugin/src/main/java/npanday/plugin/compile/AbstractCompilerMojo.java
> > URL:
> http://svn.apache.org/viewvc/incubator/npanday/trunk/plugins/maven-compile-plugin/src/main/java/npanday/plugin/compile/AbstractCompilerMojo.java?rev=1095951&r1=1095950&r2=1095951&view=diff
> >
> ==============================================================================
> > ---
> incubator/npanday/trunk/plugins/maven-compile-plugin/src/main/java/npanday/plugin/compile/AbstractCompilerMojo.java
> (original)
> > +++
> incubator/npanday/trunk/plugins/maven-compile-plugin/src/main/java/npanday/plugin/compile/AbstractCompilerMojo.java
> Fri Apr 22 15:53:58 2011
> > @@ -26,6 +26,7 @@ import npanday.executable.ExecutionExcep
> > import npanday.executable.compiler.CompilerConfig;
> > import npanday.executable.compiler.CompilerExecutable;
> > import npanday.executable.compiler.CompilerRequirement;
> > +import npanday.registry.impl.StandardRepositoryLoader;
> > import npanday.registry.RepositoryRegistry;
> > import npanday.vendor.impl.SettingsRepository;
> > import org.apache.maven.plugin.AbstractMojo;
> > @@ -1247,6 +1248,11 @@ public abstract class AbstractCompilerMo
> >
> >         File settingsFile = new File( settingsPath,
> "npanday-settings.xml" );
> >
> > +        if (!settingsFile.exists())
> > +        {
> > +            return;
> > +        }
>
> This probably needs the same changes as above.


> Are there integration tests for these changes?
>


I've added an IT for this at r1096138.


>
> - Brett
>
> --
> Brett Porter
> [email protected]
> http://brettporter.wordpress.com/
> http://au.linkedin.com/in/brettporter
>
>
>
>
>
Thanks,

--

liit

Reply via email to