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/30 11:41:31 UTC
[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
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