You are viewing a plain text version of this content. The canonical link for it is here.
Posted to npanday-dev@incubator.apache.org by Brett Porter <br...@apache.org> on 2011/04/22 23:54:50 UTC
Re: svn commit: r1095951 - in /incubator/npanday/trunk: components/dotnet-core/src/main/resources/META-INF/npanday/ plugins/maven-aspx-plugin/src/main/java/npanday/plugin/aspx/ plugins/maven-compile-plugin/src/main/java/npanday/plugin/compile/
On 23/04/2011, at 1:53 AM, apadilla@apache.org 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?
>
> 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.
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.
>
> 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?
- Brett
--
Brett Porter
brett@apache.org
http://brettporter.wordpress.com/
http://au.linkedin.com/in/brettporter
Re: svn commit: r1095951 - in /incubator/npanday/trunk: components/dotnet-core/src/main/resources/META-INF/npanday/ plugins/maven-aspx-plugin/src/main/java/npanday/plugin/aspx/ plugins/maven-compile-plugin/src/main/java/npanday/plugin/compile/
Posted by Lars Corneliussen <me...@lcorneliussen.de>.
--
Message sent from mobile device
Am 25.04.2011 um 04:29 schrieb Brett Porter <br...@apache.org>:
>
> On 23/04/2011, at 11:36 PM, Adelita Padilla wrote:
>
>> Hi Brett,
>>
>>>>
>>>> /**
>>>> + * @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.
>
> I think that property can still be confusing - I'd expect it to be a file, not a path. What do others think?
>
> - Brett
+1 on settingsFolder or refactor to settingsFile. settingsPath can be both, but since settings are expected to be in one, not multiple files, I'd also expect it to be a file path
>
> --
> Brett Porter
> brett@apache.org
> http://brettporter.wordpress.com/
> http://au.linkedin.com/in/brettporter
>
>
>
>
Re: svn commit: r1095951 - in /incubator/npanday/trunk: components/dotnet-core/src/main/resources/META-INF/npanday/ plugins/maven-aspx-plugin/src/main/java/npanday/plugin/aspx/ plugins/maven-compile-plugin/src/main/java/npanday/plugin/compile/
Posted by Brett Porter <br...@apache.org>.
On 23/04/2011, at 11:36 PM, Adelita Padilla wrote:
> Hi Brett,
>
>>>
>>> /**
>>> + * @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.
I think that property can still be confusing - I'd expect it to be a file, not a path. What do others think?
- Brett
--
Brett Porter
brett@apache.org
http://brettporter.wordpress.com/
http://au.linkedin.com/in/brettporter
Re: svn commit: r1095951 - in /incubator/npanday/trunk:
components/dotnet-core/src/main/resources/META-INF/npanday/
plugins/maven-aspx-plugin/src/main/java/npanday/plugin/aspx/ plugins/maven-compile-plugin/src/main/java/npanday/plugin/compile/
Posted by Adelita Padilla <ap...@maestrodev.com>.
Hi Brett,
On Sat, Apr 23, 2011 at 7:54 AM, Brett Porter <br...@apache.org> wrote:
>
> On 23/04/2011, at 1:53 AM, apadilla@apache.org 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
> brett@apache.org
> http://brettporter.wordpress.com/
> http://au.linkedin.com/in/brettporter
>
>
>
>
>
Thanks,
--
liit
Re: svn commit: r1095951 - in /incubator/npanday/trunk: components/dotnet-core/src/main/resources/META-INF/npanday/
plugins/maven-aspx-plugin/src/main/java/npanday/plugin/aspx/ plugins/maven-compile-plugin/src/main/java/npanday/plugin/compile/
Posted by Lars Corneliussen <me...@lcorneliussen.de>.
Am 23.04.11 01:54, schrieb Brett Porter:
> On 23/04/2011, at 1:53 AM, apadilla@apache.org 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?
>
>> 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.
>
> 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 think all path building (concatinating path segments) should be moved
to PathUtil.
>> 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?
>
> - Brett
>
> --
> Brett Porter
> brett@apache.org
> http://brettporter.wordpress.com/
> http://au.linkedin.com/in/brettporter
>
>
>
>