[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

[GitBox]

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-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org