You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/06/28 20:14:47 UTC
[GitHub] [maven-archetype] mbenson opened a new pull request #64: Support full Velocity default value expressions for required properties; properly handle ordering among same
mbenson opened a new pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64
* [ARCHETYPE-406]
* [ARCHETYPE-558]
* [ARCHETYPE-562]
Support full Velocity default value expressions for required properties; properly handle ordering among same
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] mbenson edited a comment on pull request #64: Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
mbenson edited a comment on pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#issuecomment-870010007
Also supersedes https://github.com/apache/maven-archetype/pull/61
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] elharo merged pull request #64: [ARCHETYPE-406][ARCHETYPE-558][ARCHETYPE-562] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
elharo merged pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] elharo commented on a change in pull request #64: [ARCHETYPE-406][ARCHETYPE-558][ARCHETYPE-562] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#discussion_r665417765
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -313,46 +308,24 @@ else if ( !archetypeGenerationQueryer.confirmConfiguration( archetypeConfigurati
request.setProperties( properties );
}
- private String getTransitiveDefaultValue( String defaultValue, ArchetypeConfiguration archetypeConfiguration,
- String requiredProperty, Context context )
+ private static String expandEmbeddedTemplateExpressions( String originalText, String textDescription, Context context )
{
- String result = defaultValue;
- if ( null == result )
+ if ( StringUtils.contains( originalText, "${" ) )
Review comment:
could be originalText.contains
In java 7+ we no longer need StringUtils.contains
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -368,71 +341,115 @@ private void restoreCommandLineProperties( ArchetypeConfiguration archetypeConfi
}
}
+ void setArchetypeGenerationQueryer( ArchetypeGenerationQueryer archetypeGenerationQueryer )
+ {
+ this.archetypeGenerationQueryer = archetypeGenerationQueryer;
+ }
+
public static class RequiredPropertyComparator
implements Comparator<String>
{
private final ArchetypeConfiguration archetypeConfiguration;
+ private Map<String, Set<String>> propertyReferenceMap;
+
public RequiredPropertyComparator( ArchetypeConfiguration archetypeConfiguration )
{
this.archetypeConfiguration = archetypeConfiguration;
+ propertyReferenceMap = computePropertyReferences();
}
@Override
public int compare( String left, String right )
{
- String leftDefault = archetypeConfiguration.getDefaultValue( left );
-
- if ( ( leftDefault != null ) && leftDefault.indexOf( "${" + right + "}" ) >= 0 )
- { //left contains right
+ if ( references( right, left ) )
+ {
return 1;
}
- String rightDefault = archetypeConfiguration.getDefaultValue( right );
-
- if ( ( rightDefault != null ) && rightDefault.indexOf( "${" + left + "}" ) >= 0 )
- { //right contains left
+ if ( references( left, right ) )
+ {
return -1;
}
- return comparePropertyName( left, right );
+ return Integer.compare( propertyReferenceMap.get( left ).size(), propertyReferenceMap.get( right ).size() );
}
- private int comparePropertyName( String left, String right )
+ private Map<String, Set<String>> computePropertyReferences()
{
- if ( "groupId".equals( left ) )
- {
- return -1;
- }
- if ( "groupId".equals( right ) )
- {
- return 1;
- }
- if ( "artifactId".equals( left ) )
- {
- return -1;
- }
- if ( "artifactId".equals( right ) )
+ Map<String, Set<String>> result = new HashMap<>();
+
+ List<String> requiredProperties = archetypeConfiguration.getRequiredProperties();
+
+ for ( String propertyName : requiredProperties )
{
- return 1;
+ final Set<String> referencedPropertyNames = new LinkedHashSet<>();
+
+ String defaultValue = archetypeConfiguration.getDefaultValue( propertyName );
+ if ( StringUtils.contains( defaultValue, "${" ) )
+ {
+ try
+ {
+ final boolean dumpNamespace = false;
+ SimpleNode node = RuntimeSingleton.parse(
+ new StringReader( defaultValue ), propertyName + ".default", dumpNamespace );
+ node.jjtAccept( new BaseVisitor()
+ {
+ @SuppressWarnings( "unchecked" )
+ @Override
+ public Object visit( ASTReference node, Object data )
+ {
+ ( ( Set<String> ) data ).add( node.getFirstToken().next.image );
+ return super.visit( node, data );
+ }
+ }, referencedPropertyNames );
+ }
+ catch ( ParseException e )
+ {
+ throw new IllegalStateException( "Unparsable default value for property " + propertyName, e );
+ }
+ }
+
+ result.put( propertyName, referencedPropertyNames );
}
- if ( "version".equals( left ) )
+
+ return result;
+ }
+
+ /**
+ * Semantics of this method are "references {@code targetProperty}, {@code sourceProperty} (does)."
+ *
+ * @param targetProperty
Review comment:
delete these if you're not going to provide descriptions
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -313,46 +308,24 @@ else if ( !archetypeGenerationQueryer.confirmConfiguration( archetypeConfigurati
request.setProperties( properties );
}
- private String getTransitiveDefaultValue( String defaultValue, ArchetypeConfiguration archetypeConfiguration,
- String requiredProperty, Context context )
+ private static String expandEmbeddedTemplateExpressions( String originalText, String textDescription, Context context )
Review comment:
Velocity docs call the textDescription `logTag`. Not sure if that's clearer or not. Up to you.
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -368,71 +341,115 @@ private void restoreCommandLineProperties( ArchetypeConfiguration archetypeConfi
}
}
+ void setArchetypeGenerationQueryer( ArchetypeGenerationQueryer archetypeGenerationQueryer )
+ {
+ this.archetypeGenerationQueryer = archetypeGenerationQueryer;
+ }
+
public static class RequiredPropertyComparator
implements Comparator<String>
{
private final ArchetypeConfiguration archetypeConfiguration;
+ private Map<String, Set<String>> propertyReferenceMap;
+
public RequiredPropertyComparator( ArchetypeConfiguration archetypeConfiguration )
{
this.archetypeConfiguration = archetypeConfiguration;
+ propertyReferenceMap = computePropertyReferences();
}
@Override
public int compare( String left, String right )
{
- String leftDefault = archetypeConfiguration.getDefaultValue( left );
-
- if ( ( leftDefault != null ) && leftDefault.indexOf( "${" + right + "}" ) >= 0 )
- { //left contains right
+ if ( references( right, left ) )
+ {
return 1;
}
- String rightDefault = archetypeConfiguration.getDefaultValue( right );
-
- if ( ( rightDefault != null ) && rightDefault.indexOf( "${" + left + "}" ) >= 0 )
- { //right contains left
+ if ( references( left, right ) )
+ {
return -1;
}
- return comparePropertyName( left, right );
+ return Integer.compare( propertyReferenceMap.get( left ).size(), propertyReferenceMap.get( right ).size() );
}
- private int comparePropertyName( String left, String right )
+ private Map<String, Set<String>> computePropertyReferences()
{
- if ( "groupId".equals( left ) )
- {
- return -1;
- }
- if ( "groupId".equals( right ) )
- {
- return 1;
- }
- if ( "artifactId".equals( left ) )
- {
- return -1;
- }
- if ( "artifactId".equals( right ) )
+ Map<String, Set<String>> result = new HashMap<>();
+
+ List<String> requiredProperties = archetypeConfiguration.getRequiredProperties();
+
+ for ( String propertyName : requiredProperties )
{
- return 1;
+ final Set<String> referencedPropertyNames = new LinkedHashSet<>();
+
+ String defaultValue = archetypeConfiguration.getDefaultValue( propertyName );
+ if ( StringUtils.contains( defaultValue, "${" ) )
Review comment:
defaultValue.contains?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] mbenson commented on pull request #64: [ARCHETYPE-406] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
mbenson commented on pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#issuecomment-872270491
I believe I have addressed all requested modifications, but note that the ML received no notification of same. Thanks for your consideration of these changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] mbenson commented on pull request #64: [ARCHETYPE-406][ARCHETYPE-558][ARCHETYPE-562] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
mbenson commented on pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#issuecomment-874863683
Looks like that succeeded, yes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] mbenson commented on pull request #64: [ARCHETYPE-406] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
mbenson commented on pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#issuecomment-871372728
Hi @elharo ... incidentally in my semester of Java your book was my textbook... which ultimately led me to a 20-year-plus-and-running career, so thanks!
Several of your notes were made against code that existed before I started making changes. I don't mind making the changes, but could you confirm that you prefer to see the changes made as part of this request? In the meantime I'll work on addressing the comments that apply to my own work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] mbenson commented on a change in pull request #64: [ARCHETYPE-406][ARCHETYPE-558][ARCHETYPE-562] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
mbenson commented on a change in pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#discussion_r664731952
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -313,46 +311,23 @@ else if ( !archetypeGenerationQueryer.confirmConfiguration( archetypeConfigurati
request.setProperties( properties );
}
- private String getTransitiveDefaultValue( String defaultValue, ArchetypeConfiguration archetypeConfiguration,
- String requiredProperty, Context context )
+ private String getTransitiveDefaultValue( String defaultValue, String requiredProperty, Context context )
{
- String result = defaultValue;
- if ( null == result )
+ if ( StringUtils.contains( defaultValue, "${" ) )
{
- return null;
- }
- for ( String property : archetypeConfiguration.getRequiredProperties() )
- {
- if ( result.indexOf( "${" + property + "}" ) >= 0 )
+ try ( StringWriter stringWriter = new StringWriter() )
{
-
- result = StringUtils.replace( result, "${" + property + "}",
- archetypeConfiguration.getProperty( property ) );
+ Velocity.evaluate( context, stringWriter, requiredProperty, defaultValue );
Review comment:
This is the default value of some required property of the archetype, which in this context has been determined to appear to contain expansion syntax (`${`) indicative of the whole being intended to be interpreted as a Velocity expression. Does that make sense?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] elharo commented on a change in pull request #64: [ARCHETYPE-406] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#discussion_r661379670
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -313,46 +320,22 @@ else if ( !archetypeGenerationQueryer.confirmConfiguration( archetypeConfigurati
request.setProperties( properties );
}
- private String getTransitiveDefaultValue( String defaultValue, ArchetypeConfiguration archetypeConfiguration,
- String requiredProperty, Context context )
+ private String getTransitiveDefaultValue( String defaultValue, String requiredProperty, Context context )
{
- String result = defaultValue;
- if ( null == result )
+ if ( StringUtils.contains( defaultValue, "${" ) )
{
- return null;
- }
- for ( String property : archetypeConfiguration.getRequiredProperties() )
- {
- if ( result.indexOf( "${" + property + "}" ) >= 0 )
+ try ( StringWriter stringWriter = new StringWriter() )
+ {
+ Velocity.evaluate( context, stringWriter, requiredProperty, defaultValue );
+ return stringWriter.toString();
+ }
+ catch ( Exception ex )
Review comment:
can this exception be made more specific?
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -368,71 +351,113 @@ private void restoreCommandLineProperties( ArchetypeConfiguration archetypeConfi
}
}
+ void setArchetypeGenerationQueryer( ArchetypeGenerationQueryer archetypeGenerationQueryer )
+ {
+ this.archetypeGenerationQueryer = archetypeGenerationQueryer;
+ }
+
public static class RequiredPropertyComparator
implements Comparator<String>
{
private final ArchetypeConfiguration archetypeConfiguration;
+ private Map<String, Set<String>> propertyReferenceMap;
+
public RequiredPropertyComparator( ArchetypeConfiguration archetypeConfiguration )
{
this.archetypeConfiguration = archetypeConfiguration;
+ propertyReferenceMap = computePropertyReferences();
}
@Override
public int compare( String left, String right )
{
- String leftDefault = archetypeConfiguration.getDefaultValue( left );
-
- if ( ( leftDefault != null ) && leftDefault.indexOf( "${" + right + "}" ) >= 0 )
- { //left contains right
+ if ( references( right, left ) )
+ {
return 1;
}
- String rightDefault = archetypeConfiguration.getDefaultValue( right );
-
- if ( ( rightDefault != null ) && rightDefault.indexOf( "${" + left + "}" ) >= 0 )
- { //right contains left
+ if ( references( left, right ) )
+ {
return -1;
}
- return comparePropertyName( left, right );
+ return Integer.compare( propertyReferenceMap.get( left ).size(), propertyReferenceMap.get( right ).size() );
}
- private int comparePropertyName( String left, String right )
+ private Map<String, Set<String>> computePropertyReferences()
{
- if ( "groupId".equals( left ) )
- {
- return -1;
- }
- if ( "groupId".equals( right ) )
- {
- return 1;
- }
- if ( "artifactId".equals( left ) )
- {
- return -1;
- }
- if ( "artifactId".equals( right ) )
+ Map<String, Set<String>> result = new HashMap<>();
+
+ List<String> requiredProperties = archetypeConfiguration.getRequiredProperties();
+
+ for ( String propertyName : requiredProperties )
{
- return 1;
+ final Set<String> referencedPropertyNames = new LinkedHashSet<>();
+
+ String defaultValue = archetypeConfiguration.getDefaultValue( propertyName );
+ if ( StringUtils.contains( defaultValue, "${" ) )
+ {
+ try
+ {
+ final boolean dumpNamespace = false;
+ SimpleNode node = RuntimeSingleton.parse(
+ new StringReader( defaultValue ), propertyName + ".default", dumpNamespace );
+ node.jjtAccept( new BaseVisitor()
+ {
+ @SuppressWarnings( "unchecked" )
+ @Override
+ public Object visit( ASTReference node, Object data )
+ {
+ ( ( Set<String> ) data ).add( node.getFirstToken().next.image );
+ return super.visit( node, data );
+ }
+ }, referencedPropertyNames );
+ }
+ catch ( ParseException e )
+ {
+ }
+ }
+
+ result.put( propertyName, referencedPropertyNames );
}
- if ( "version".equals( left ) )
+
+ return result;
+ }
+
+ /**
+ * Yoda semantics.
Review comment:
I don't understand this comment. Please rephrase.
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -168,65 +183,54 @@ else if ( archetypeArtifactManager.isOldArchetype( ad.getGroupId(), ad.getArtifa
context.put( Constants.VERSION, ad.getVersion() );
while ( !confirmed )
{
- List<String> propertiesRequired = archetypeConfiguration.getRequiredProperties();
- getLogger().debug( "Required properties before content sort: " + propertiesRequired );
- Collections.sort( propertiesRequired, new RequiredPropertyComparator( archetypeConfiguration ) );
- getLogger().debug( "Required properties after content sort: " + propertiesRequired );
-
if ( !archetypeConfiguration.isConfigured() )
{
for ( String requiredProperty : propertiesRequired )
{
+ String value;
+
if ( !archetypeConfiguration.isConfigured( requiredProperty ) )
{
- if ( "package".equals( requiredProperty ) )
+ if ( Constants.PACKAGE.equals( requiredProperty ) )
{
// if the asked property is 'package', then
// use its default and if not defined,
// use the 'groupId' property value.
String packageDefault = archetypeConfiguration.getDefaultValue( requiredProperty );
- packageDefault = ( null == packageDefault || "".equals( packageDefault ) )
- ? archetypeConfiguration.getProperty( "groupId" )
- : archetypeConfiguration.getDefaultValue( requiredProperty );
+ if ( StringUtils.isEmpty( packageDefault ) )
+ {
+ packageDefault = archetypeConfiguration.getProperty( Constants.GROUP_ID );
+ }
- String value =
- getTransitiveDefaultValue( packageDefault, archetypeConfiguration, requiredProperty,
- context );
+ value = getTransitiveDefaultValue( packageDefault, requiredProperty, context );
value = archetypeGenerationQueryer.getPropertyValue( requiredProperty, value, null );
-
- archetypeConfiguration.setProperty( requiredProperty, value );
-
- context.put( Constants.PACKAGE, value );
}
else
{
- String value = archetypeConfiguration.getDefaultValue( requiredProperty );
+ value = archetypeConfiguration.getDefaultValue( requiredProperty );
- value = getTransitiveDefaultValue( value, archetypeConfiguration, requiredProperty,
- context );
+ value = getTransitiveDefaultValue( value, requiredProperty, context );
Review comment:
it would be much clearer not to reuse the local variables
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -368,71 +351,113 @@ private void restoreCommandLineProperties( ArchetypeConfiguration archetypeConfi
}
}
+ void setArchetypeGenerationQueryer( ArchetypeGenerationQueryer archetypeGenerationQueryer )
+ {
+ this.archetypeGenerationQueryer = archetypeGenerationQueryer;
+ }
+
public static class RequiredPropertyComparator
implements Comparator<String>
{
private final ArchetypeConfiguration archetypeConfiguration;
+ private Map<String, Set<String>> propertyReferenceMap;
+
public RequiredPropertyComparator( ArchetypeConfiguration archetypeConfiguration )
{
this.archetypeConfiguration = archetypeConfiguration;
+ propertyReferenceMap = computePropertyReferences();
}
@Override
public int compare( String left, String right )
{
- String leftDefault = archetypeConfiguration.getDefaultValue( left );
-
- if ( ( leftDefault != null ) && leftDefault.indexOf( "${" + right + "}" ) >= 0 )
- { //left contains right
+ if ( references( right, left ) )
+ {
return 1;
}
- String rightDefault = archetypeConfiguration.getDefaultValue( right );
-
- if ( ( rightDefault != null ) && rightDefault.indexOf( "${" + left + "}" ) >= 0 )
- { //right contains left
+ if ( references( left, right ) )
+ {
return -1;
}
- return comparePropertyName( left, right );
+ return Integer.compare( propertyReferenceMap.get( left ).size(), propertyReferenceMap.get( right ).size() );
}
- private int comparePropertyName( String left, String right )
+ private Map<String, Set<String>> computePropertyReferences()
{
- if ( "groupId".equals( left ) )
- {
- return -1;
- }
- if ( "groupId".equals( right ) )
- {
- return 1;
- }
- if ( "artifactId".equals( left ) )
- {
- return -1;
- }
- if ( "artifactId".equals( right ) )
+ Map<String, Set<String>> result = new HashMap<>();
+
+ List<String> requiredProperties = archetypeConfiguration.getRequiredProperties();
+
+ for ( String propertyName : requiredProperties )
{
- return 1;
+ final Set<String> referencedPropertyNames = new LinkedHashSet<>();
+
+ String defaultValue = archetypeConfiguration.getDefaultValue( propertyName );
+ if ( StringUtils.contains( defaultValue, "${" ) )
+ {
+ try
+ {
+ final boolean dumpNamespace = false;
+ SimpleNode node = RuntimeSingleton.parse(
+ new StringReader( defaultValue ), propertyName + ".default", dumpNamespace );
+ node.jjtAccept( new BaseVisitor()
+ {
+ @SuppressWarnings( "unchecked" )
+ @Override
+ public Object visit( ASTReference node, Object data )
+ {
+ ( ( Set<String> ) data ).add( node.getFirstToken().next.image );
+ return super.visit( node, data );
+ }
+ }, referencedPropertyNames );
+ }
+ catch ( ParseException e )
Review comment:
if an empty catch block is really OK here, please add a comment explaining why
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -159,6 +169,11 @@ else if ( archetypeArtifactManager.isOldArchetype( ad.getGroupId(), ad.getArtifa
throw new ArchetypeGenerationConfigurationFailure( "The defined artifact is not an archetype" );
}
+ List<String> propertiesRequired = archetypeConfiguration.getRequiredProperties();
+ getLogger().debug( "Required properties before content sort: " + propertiesRequired );
Review comment:
please take these logging statements out before committing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] elharo commented on a change in pull request #64: [ARCHETYPE-406][ARCHETYPE-558][ARCHETYPE-562] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#discussion_r664784855
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -313,46 +311,25 @@ else if ( !archetypeGenerationQueryer.confirmConfiguration( archetypeConfigurati
request.setProperties( properties );
}
- private String getTransitiveDefaultValue( String defaultValue, ArchetypeConfiguration archetypeConfiguration,
- String requiredProperty, Context context )
+ private String getTransitiveDefaultValue( String defaultValue, String requiredProperty, Context context )
Review comment:
This method name is unclear. `evaluateTemplate` would be clearer.
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -313,46 +311,25 @@ else if ( !archetypeGenerationQueryer.confirmConfiguration( archetypeConfigurati
request.setProperties( properties );
}
- private String getTransitiveDefaultValue( String defaultValue, ArchetypeConfiguration archetypeConfiguration,
- String requiredProperty, Context context )
+ private String getTransitiveDefaultValue( String defaultValue, String requiredProperty, Context context )
{
- String result = defaultValue;
- if ( null == result )
+ if ( StringUtils.contains( defaultValue, "${" ) )
{
- return null;
- }
- for ( String property : archetypeConfiguration.getRequiredProperties() )
- {
- if ( result.indexOf( "${" + property + "}" ) >= 0 )
+ try ( StringWriter stringWriter = new StringWriter() )
{
-
- result = StringUtils.replace( result, "${" + property + "}",
- archetypeConfiguration.getProperty( property ) );
+ Velocity.evaluate( context, stringWriter, requiredProperty, defaultValue );
+ return stringWriter.toString();
+ }
+ catch ( IOException ex )
+ {
+ // closing StringWriter shouldn't actually generate any exception
+ throw new RuntimeException(
+ String.format("Exception closing %s", StringWriter.class.getSimpleName()), ex);
Review comment:
no need to use String.format or getClass here. This is always a constant string.
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -313,46 +311,23 @@ else if ( !archetypeGenerationQueryer.confirmConfiguration( archetypeConfigurati
request.setProperties( properties );
}
- private String getTransitiveDefaultValue( String defaultValue, ArchetypeConfiguration archetypeConfiguration,
- String requiredProperty, Context context )
+ private String getTransitiveDefaultValue( String defaultValue, String requiredProperty, Context context )
{
- String result = defaultValue;
- if ( null == result )
+ if ( StringUtils.contains( defaultValue, "${" ) )
{
- return null;
- }
- for ( String property : archetypeConfiguration.getRequiredProperties() )
- {
- if ( result.indexOf( "${" + property + "}" ) >= 0 )
+ try ( StringWriter stringWriter = new StringWriter() )
{
-
- result = StringUtils.replace( result, "${" + property + "}",
- archetypeConfiguration.getProperty( property ) );
+ Velocity.evaluate( context, stringWriter, requiredProperty, defaultValue );
Review comment:
Not, not really. That's where it came from, but it's not what it is inside this method or inside Velocity.evaluate. It's a template to be interpreted.
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -313,46 +311,25 @@ else if ( !archetypeGenerationQueryer.confirmConfiguration( archetypeConfigurati
request.setProperties( properties );
}
- private String getTransitiveDefaultValue( String defaultValue, ArchetypeConfiguration archetypeConfiguration,
- String requiredProperty, Context context )
+ private String getTransitiveDefaultValue( String defaultValue, String requiredProperty, Context context )
Review comment:
looks like this could be static since it doesn't depend on instance state (unless I'm missing something)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] mbenson commented on pull request #64: [ARCHETYPE-406][ARCHETYPE-558][ARCHETYPE-562] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
mbenson commented on pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#issuecomment-875738621
Thanks @elharo ! What's next?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] elharo commented on a change in pull request #64: [ARCHETYPE-406][ARCHETYPE-558][ARCHETYPE-562] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#discussion_r664676528
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -313,46 +311,23 @@ else if ( !archetypeGenerationQueryer.confirmConfiguration( archetypeConfigurati
request.setProperties( properties );
}
- private String getTransitiveDefaultValue( String defaultValue, ArchetypeConfiguration archetypeConfiguration,
- String requiredProperty, Context context )
+ private String getTransitiveDefaultValue( String defaultValue, String requiredProperty, Context context )
{
- String result = defaultValue;
- if ( null == result )
+ if ( StringUtils.contains( defaultValue, "${" ) )
{
- return null;
- }
- for ( String property : archetypeConfiguration.getRequiredProperties() )
- {
- if ( result.indexOf( "${" + property + "}" ) >= 0 )
+ try ( StringWriter stringWriter = new StringWriter() )
{
-
- result = StringUtils.replace( result, "${" + property + "}",
- archetypeConfiguration.getProperty( property ) );
+ Velocity.evaluate( context, stringWriter, requiredProperty, defaultValue );
+ return stringWriter.toString();
+ }
+ catch ( IOException ex )
+ {
+ // closing StringWriter shouldn't actually generate any exception
Review comment:
Yes, but wrap in a runtime exception just in case.
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -313,46 +311,23 @@ else if ( !archetypeGenerationQueryer.confirmConfiguration( archetypeConfigurati
request.setProperties( properties );
}
- private String getTransitiveDefaultValue( String defaultValue, ArchetypeConfiguration archetypeConfiguration,
- String requiredProperty, Context context )
+ private String getTransitiveDefaultValue( String defaultValue, String requiredProperty, Context context )
{
- String result = defaultValue;
- if ( null == result )
+ if ( StringUtils.contains( defaultValue, "${" ) )
{
- return null;
- }
- for ( String property : archetypeConfiguration.getRequiredProperties() )
- {
- if ( result.indexOf( "${" + property + "}" ) >= 0 )
+ try ( StringWriter stringWriter = new StringWriter() )
{
-
- result = StringUtils.replace( result, "${" + property + "}",
- archetypeConfiguration.getProperty( property ) );
+ Velocity.evaluate( context, stringWriter, requiredProperty, defaultValue );
Review comment:
is defaultValue correctly named? This looks like a template, not a default value, but I'm not a velocity expert.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] mbenson edited a comment on pull request #64: Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
mbenson edited a comment on pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#issuecomment-870010007
Also supersedes https://github.com/apache/maven-archetype/pull/61
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] mbenson commented on a change in pull request #64: [ARCHETYPE-406][ARCHETYPE-558][ARCHETYPE-562] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
mbenson commented on a change in pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#discussion_r665435676
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -368,71 +341,115 @@ private void restoreCommandLineProperties( ArchetypeConfiguration archetypeConfi
}
}
+ void setArchetypeGenerationQueryer( ArchetypeGenerationQueryer archetypeGenerationQueryer )
+ {
+ this.archetypeGenerationQueryer = archetypeGenerationQueryer;
+ }
+
public static class RequiredPropertyComparator
implements Comparator<String>
{
private final ArchetypeConfiguration archetypeConfiguration;
+ private Map<String, Set<String>> propertyReferenceMap;
+
public RequiredPropertyComparator( ArchetypeConfiguration archetypeConfiguration )
{
this.archetypeConfiguration = archetypeConfiguration;
+ propertyReferenceMap = computePropertyReferences();
}
@Override
public int compare( String left, String right )
{
- String leftDefault = archetypeConfiguration.getDefaultValue( left );
-
- if ( ( leftDefault != null ) && leftDefault.indexOf( "${" + right + "}" ) >= 0 )
- { //left contains right
+ if ( references( right, left ) )
+ {
return 1;
}
- String rightDefault = archetypeConfiguration.getDefaultValue( right );
-
- if ( ( rightDefault != null ) && rightDefault.indexOf( "${" + left + "}" ) >= 0 )
- { //right contains left
+ if ( references( left, right ) )
+ {
return -1;
}
- return comparePropertyName( left, right );
+ return Integer.compare( propertyReferenceMap.get( left ).size(), propertyReferenceMap.get( right ).size() );
}
- private int comparePropertyName( String left, String right )
+ private Map<String, Set<String>> computePropertyReferences()
{
- if ( "groupId".equals( left ) )
- {
- return -1;
- }
- if ( "groupId".equals( right ) )
- {
- return 1;
- }
- if ( "artifactId".equals( left ) )
- {
- return -1;
- }
- if ( "artifactId".equals( right ) )
+ Map<String, Set<String>> result = new HashMap<>();
+
+ List<String> requiredProperties = archetypeConfiguration.getRequiredProperties();
+
+ for ( String propertyName : requiredProperties )
{
- return 1;
+ final Set<String> referencedPropertyNames = new LinkedHashSet<>();
+
+ String defaultValue = archetypeConfiguration.getDefaultValue( propertyName );
+ if ( StringUtils.contains( defaultValue, "${" ) )
Review comment:
`null` safety
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] mbenson commented on pull request #64: Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
mbenson commented on pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#issuecomment-870010007
Also supercedes https://github.com/apache/maven-archetype/pull/61
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] elharo commented on pull request #64: [ARCHETYPE-406][ARCHETYPE-558][ARCHETYPE-562] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
elharo commented on pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#issuecomment-875643202
checking again https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-archetype/job/test3/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] elharo commented on a change in pull request #64: [ARCHETYPE-406] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#discussion_r661384190
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -368,71 +351,113 @@ private void restoreCommandLineProperties( ArchetypeConfiguration archetypeConfi
}
}
+ void setArchetypeGenerationQueryer( ArchetypeGenerationQueryer archetypeGenerationQueryer )
+ {
+ this.archetypeGenerationQueryer = archetypeGenerationQueryer;
+ }
+
public static class RequiredPropertyComparator
implements Comparator<String>
{
private final ArchetypeConfiguration archetypeConfiguration;
+ private Map<String, Set<String>> propertyReferenceMap;
+
public RequiredPropertyComparator( ArchetypeConfiguration archetypeConfiguration )
{
this.archetypeConfiguration = archetypeConfiguration;
+ propertyReferenceMap = computePropertyReferences();
}
@Override
public int compare( String left, String right )
{
- String leftDefault = archetypeConfiguration.getDefaultValue( left );
-
- if ( ( leftDefault != null ) && leftDefault.indexOf( "${" + right + "}" ) >= 0 )
- { //left contains right
+ if ( references( right, left ) )
+ {
return 1;
}
- String rightDefault = archetypeConfiguration.getDefaultValue( right );
-
- if ( ( rightDefault != null ) && rightDefault.indexOf( "${" + left + "}" ) >= 0 )
- { //right contains left
+ if ( references( left, right ) )
+ {
return -1;
}
- return comparePropertyName( left, right );
+ return Integer.compare( propertyReferenceMap.get( left ).size(), propertyReferenceMap.get( right ).size() );
}
- private int comparePropertyName( String left, String right )
+ private Map<String, Set<String>> computePropertyReferences()
{
- if ( "groupId".equals( left ) )
- {
- return -1;
- }
- if ( "groupId".equals( right ) )
- {
- return 1;
- }
- if ( "artifactId".equals( left ) )
- {
- return -1;
- }
- if ( "artifactId".equals( right ) )
+ Map<String, Set<String>> result = new HashMap<>();
+
+ List<String> requiredProperties = archetypeConfiguration.getRequiredProperties();
+
+ for ( String propertyName : requiredProperties )
{
- return 1;
+ final Set<String> referencedPropertyNames = new LinkedHashSet<>();
+
+ String defaultValue = archetypeConfiguration.getDefaultValue( propertyName );
+ if ( StringUtils.contains( defaultValue, "${" ) )
+ {
+ try
+ {
+ final boolean dumpNamespace = false;
+ SimpleNode node = RuntimeSingleton.parse(
+ new StringReader( defaultValue ), propertyName + ".default", dumpNamespace );
+ node.jjtAccept( new BaseVisitor()
+ {
+ @SuppressWarnings( "unchecked" )
+ @Override
+ public Object visit( ASTReference node, Object data )
+ {
+ ( ( Set<String> ) data ).add( node.getFirstToken().next.image );
+ return super.visit( node, data );
+ }
+ }, referencedPropertyNames );
+ }
+ catch ( ParseException e )
+ {
+ }
+ }
+
+ result.put( propertyName, referencedPropertyNames );
}
- if ( "version".equals( left ) )
+
+ return result;
+ }
+
+ /**
+ * Yoda semantics.
+ * @param targetProperty
+ * @param sourceProperty
+ * @return {@code boolean}
+ */
+ private boolean references( String targetProperty, String sourceProperty )
+ {
+ if ( targetProperty.equals( sourceProperty ) )
{
- return -1;
+ return false;
}
- if ( "version".equals( right ) )
+ synchronized ( this )
{
- return 1;
+ if ( ! propertyReferenceMap.containsKey( sourceProperty ) )
+ // something has changed
+ {
+ this.propertyReferenceMap = computePropertyReferences();
+ }
}
- if ( "package".equals( left ) )
+ Set<String> references = propertyReferenceMap.get( sourceProperty );
Review comment:
It's mildly confusing to have a local variable and method with the same name. Can you rename one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] mbenson commented on a change in pull request #64: [ARCHETYPE-406][ARCHETYPE-558][ARCHETYPE-562] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
mbenson commented on a change in pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#discussion_r665437580
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -313,46 +308,24 @@ else if ( !archetypeGenerationQueryer.confirmConfiguration( archetypeConfigurati
request.setProperties( properties );
}
- private String getTransitiveDefaultValue( String defaultValue, ArchetypeConfiguration archetypeConfiguration,
- String requiredProperty, Context context )
+ private static String expandEmbeddedTemplateExpressions( String originalText, String textDescription, Context context )
{
- String result = defaultValue;
- if ( null == result )
+ if ( StringUtils.contains( originalText, "${" ) )
Review comment:
`null` safety
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] elharo commented on pull request #64: [ARCHETYPE-406][ARCHETYPE-558][ARCHETYPE-562] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
elharo commented on pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#issuecomment-875786039
one more time: https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-archetype/job/test4/
if this passes and there are no more commits, I'll merge
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] mbenson commented on pull request #64: Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
mbenson commented on pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#issuecomment-870010007
Also supercedes https://github.com/apache/maven-archetype/pull/61
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] mbenson commented on a change in pull request #64: [ARCHETYPE-406][ARCHETYPE-558][ARCHETYPE-562] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
mbenson commented on a change in pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#discussion_r665436637
##########
File path: maven-archetype-plugin/src/main/java/org/apache/maven/archetype/ui/generation/DefaultArchetypeGenerationConfigurator.java
##########
@@ -313,46 +308,24 @@ else if ( !archetypeGenerationQueryer.confirmConfiguration( archetypeConfigurati
request.setProperties( properties );
}
- private String getTransitiveDefaultValue( String defaultValue, ArchetypeConfiguration archetypeConfiguration,
- String requiredProperty, Context context )
+ private static String expandEmbeddedTemplateExpressions( String originalText, String textDescription, Context context )
Review comment:
I kind of thought to use these descriptions as a gradual "bridge" over to Velocity's conceptual model.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] mbenson commented on pull request #64: [ARCHETYPE-406][ARCHETYPE-558][ARCHETYPE-562] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
mbenson commented on pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#issuecomment-875635447
Addressed latest comments... unfortunately I seem to lack permission to re-trigger the CI build.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-archetype] elharo commented on pull request #64: [ARCHETYPE-406][ARCHETYPE-558][ARCHETYPE-562] Support full Velocity default value expressions for required properties; properly handle ordering among same
Posted by GitBox <gi...@apache.org>.
elharo commented on pull request #64:
URL: https://github.com/apache/maven-archetype/pull/64#issuecomment-875746655
I should the final PR in jenkins and merge it if it passs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org