[GitHub] [maven-release] elharo commented on a diff in pull request #196: [MRELEASE-1054] Support for excluding submodules changes.

["elharo (via GitHub)" <gi...@apache.org>]

elharo commented on code in PR #196:
URL: https://github.com/apache/maven-release/pull/196#discussion_r1292889605


##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractMapVersionsPhase.java:
##########
@@ -39,6 +43,7 @@
 import org.apache.maven.shared.release.versions.VersionParseException;
 import org.codehaus.plexus.components.interactivity.Prompter;
 import org.codehaus.plexus.components.interactivity.PrompterException;
+import org.codehaus.plexus.util.SelectorUtils;

Review Comment:
   I'd like to avoid addition al plexus dependencies if at all possible.



##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractRewritePomsPhase.java:
##########
@@ -142,6 +147,15 @@ public ReleaseResult execute(
             throws ReleaseExecutionException, ReleaseFailureException {
         ReleaseResult result = new ReleaseResult();
 
+        List<String> additionalExcludes = releaseDescriptor.getCheckModificationExcludes();
+
+        if (additionalExcludes != null) {
+            for (String additionalExclude : additionalExcludes) {
+                exclusionPatterns.add(
+                        additionalExclude.replace("\\", File.separator).replace("/", File.separator));

Review Comment:
   same as above



##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractMapVersionsPhase.java:
##########
@@ -118,6 +125,15 @@ public ReleaseResult execute(
             throws ReleaseExecutionException {
         ReleaseResult result = new ReleaseResult();
 
+        List<String> additionalExcludes = releaseDescriptor.getCheckModificationExcludes();
+
+        if (additionalExcludes != null) {
+            for (String additionalExclude : additionalExcludes) {
+                exclusionPatterns.add(
+                        additionalExclude.replace("\\", File.separator).replace("/", File.separator));

Review Comment:
   File.separator is platform dependent, but I'm not sure it should be here. That is, exclusions should be specified in a platform independent, standard way. I could be wrong about that but if I am, please elaborate in detail on how this works with references to docs. 



##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractRewritePomsPhase.java:
##########
@@ -199,13 +213,18 @@ private void transform(
 
         for (MavenProject project : reactorProjects) {
             URI pom = project.getFile().toURI();
-            logInfo(
-                    result,
-                    "Transforming " + root.relativize(pom).getPath() + ' '
-                            + buffer().project(project.getArtifactId()) + " '" + project.getName() + "'"
-                            + (simulate ? " with ." + getPomSuffix() + " suffix" : "") + "...");
+            final String path = root.relativize(pom).getPath();
+
+            if (exclusionPatterns.stream()
+                    .noneMatch(exclusionPattern -> SelectorUtils.matchPath(exclusionPattern, path))) {
+                logInfo(

Review Comment:
   why log this? debug level at most



##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractMapVersionsPhase.java:
##########
@@ -165,17 +181,24 @@ public ReleaseResult execute(
                 }
             }
         } else {
+            URI root = rootProject.getBasedir().toURI();
             for (MavenProject project : reactorProjects) {
                 String projectId = ArtifactUtils.versionlessKey(project.getGroupId(), project.getArtifactId());
 
                 String nextVersion = resolveNextVersion(project, projectId, releaseDescriptor, releaseEnvironment);
 
-                if (!convertToSnapshot) {
-                    releaseDescriptor.addReleaseVersion(projectId, nextVersion);
-                } else if (releaseDescriptor.isBranchCreation() && convertToBranch) {
-                    releaseDescriptor.addReleaseVersion(projectId, nextVersion);
-                } else {
-                    releaseDescriptor.addDevelopmentVersion(projectId, nextVersion);
+                URI pom = project.getFile().toURI();
+                final String path = root.relativize(pom).getPath();
+
+                if (exclusionPatterns.stream()
+                        .noneMatch(exclusionPattern -> SelectorUtils.matchPath(exclusionPattern, path))) {

Review Comment:
   a basic loop with no lambdas would be easier to read



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