[GitHub] [maven-release] michael-o commented on a diff in pull request #104: [MRELEASE-1078] Support version policy to read and use SCM tags and commits

["michael-o (via GitHub)" <gi...@apache.org>]

michael-o commented on code in PR #104:
URL: https://github.com/apache/maven-release/pull/104#discussion_r1127652273


##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/config/ReleaseDescriptorBuilder.java:
##########
@@ -401,6 +401,18 @@ public ReleaseDescriptorBuilder setProjectVersionPolicyId( String projectVersion
         return this;
     }
 
+    /**
+     * <p>setProjectVersionPolicyConfig.</p>
+     *
+     * @param setProjectVersionPolicyConfig a {@link java.lang.String} object
+     * @return a {@link org.apache.maven.shared.release.config.ReleaseDescriptorBuilder} object
+     */
+    public ReleaseDescriptorBuilder setProjectVersionPolicyConfig( String setProjectVersionPolicyConfig )
+    {
+        releaseDescriptor.setProjectVersionPolicyConfig( setProjectVersionPolicyConfig );
+        return this;

Review Comment:
   This I don't understand. If the release descriptor supports an object, why not pass the original one instead of a string representation?



##########
maven-release-api/src/main/java/org/apache/maven/shared/release/config/ReleaseDescriptor.java:
##########
@@ -426,6 +426,13 @@
      */
     String getProjectVersionPolicyId();
 
+    /**
+     * Get the (optional) config for the VersionPolicy implementation used to calculate the project versions.
+     *
+     * @return The parsed XML of the provided config (an instance of XmlPlexusConfiguration) or null.
+     */
+    Object getProjectVersionPolicyConfig();
+

Review Comment:
   Does not really make sense to tie it to XML, consider someone is using this outside of Maven realm and uses real objects throughout. Why not create a simple interface `VersionPolicyConfig` which has a `#serialize()` method. For the PlexusDom we can create an impl which will delegate to `#toString()`, thus we can cleanly separate between Maven Release and Maven Core usage. WDYT?`



-- 
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