Repository: vackosar/gitflow-incremental-builder
Pull request #234 (2020-08-23 21:48:59)

Rename property 'uncommited' to 'uncommitted'
FYI @mickroll, this is an example of how we can fix typos etc. without immediately breaking clients.

famod: ...although I have to say that this does not cover plugin config backward compatibility.
The only thing we could do would be to generate a "parameter alias". I'll have a timeboxed look at it...
mickroll: tl;dr having a 'parameter alias' sounds a bit too comfy. Maybe checking for unsupported config properties would be a better solution?

> The only thing we could do would be to generate a "parameter alias". I'll have a timeboxed look at it...

Fixing that typo and moving from `gib.enabled` to `gib.disabled` are API breaking changes. But, i would call them 'mild' changes:
1. the main config of the build extension is likely to reside at one place for a project, in the root pom.xml
2. selectively disabling the plugin may be used in central/local build scripts

Hence, adjusting tools for these changes shouldn't be a big task on the users side.

Having the comfort for a transition from `gib.enabled` to `gib.disabled` within a version is great. But i don't really see the need for it for fixing a typo in the property name. Simply scheduling it for the next minor version should suffice.

A second thought: is there a detection for unsupported properties (like `gib.nonexistent`)? This might be a good help for users to find broken configurations, instead of introducing (and supporting) parameter aliases. 
famod: Ah, saw your comment too late. Yes we are already checking for invalid properties.

I agree that renaming a simple property should not be a big deal but I consider a softer approach like a warning + grace period until the next release more user friendly. Both approaches have pros and cons.

I have pushed the alias thing, it was rather easy. One side effect is that in Eclipse you will be offered both variants, which is not really what we want.
I think I'll have a another look tomorrow to check whether an "unknown" property still ends up in the plugin properties. Not so sure about that. Maven might drop it.
mickroll: > Ah, saw your comment too late. Yes we are already checking for invalid properties.

Great!

> I have pushed the alias thing, it was rather easy. One side effect is that in Eclipse you will be offered both variants, which is not really what we want.

Hmm, maybe a `@Deprecated` is honored? Would need to check the generated Mojo metadata, can't from where i am (only have access to my phone currently). But a deprecation remark in the property description text would be user friendly.

> I think I'll have a another look tomorrow to check whether an "unknown" property still ends up in the plugin properties. Not so sure about that. Maven might drop it.

Slight misunderstanding here: i wasn't talking about plugin configuration (i think maven should not only drop them, but stop and print an error. At least for a 'real' plugin). I thought about validating project/system property names within the `gib.*` 'namespace'.
famod: > i think maven should not only drop them, but stop and print an error. At least for a 'real' plugin.

For Maven, GIB as a plugin just looks like a regular plugin. So that does not make a difference.
I just tested this and I recalled it correctly: Maven does _not_ complain about "unknown" properties.

Luckily, Maven does _not_ drop such a unknown property either! So nothing really has to be done in GIB, the warning will be logged if you use the deprecated name, since the property is added to the plugin properties by Maven and GIB can read it properly. The alias is not relevant for this.

Long story short: I should have tested this properly in the first place. 😉 
I'll remove the alias stuff and then it is good to go...

Patch:
diff --git a/README.md b/README.md
index 5ba789c..774d1e0 100644
--- a/README.md
+++ b/README.md
@@ -33,7 +33,7 @@ This extension is **not limited to Git Flow setups!** The [extensive configurati
   - [gib.fetchBaseBranch](#gibfetchbasebranch)
   - [gib.useJschAgentProxy](#gibusejschagentproxy)
   - [gib.compareToMergeBase](#gibcomparetomergebase)
-  - [gib.uncommited](#gibuncommited)
+  - [gib.uncommitted](#gibuncommitted)
   - [gib.untracked](#gibuntracked)
   - [gib.excludePathRegex](#gibexcludePathRegex)
   - [gib.includePathRegex](#gibincludepathregex)
@@ -321,7 +321,7 @@ Maven pom properties configuration with default values is below:
     <gib.fetchBaseBranch>false</gib.fetchBaseBranch>                                   <!-- or -Dgib.fbb=...   -->
     <gib.useJschAgentProxy>true</gib.useJschAgentProxy>                                <!-- or -Dgib.ujap=...  -->
     <gib.compareToMergeBase>true</gib.compareToMergeBase>                              <!-- or -Dgib.ctmb=...  -->
-    <gib.uncommited>true</gib.uncommited>                                              <!-- or -Dgib.uc=...    -->
+    <gib.uncommitted>true</gib.uncommitted>                                            <!-- or -Dgib.uc=...    -->
     <gib.untracked>true</gib.untracked>                                                <!-- or -Dgib.ut=...    -->
     <gib.excludePathRegex></gib.excludePathRegex>                                      <!-- or -Dgib.epr=...   -->
     <gib.includePathRegex></gib.includePathRegex>                                      <!-- or -Dgib.ipr=...   -->
@@ -354,16 +354,16 @@ to put the properties into the plugin `<configuration>`-section, but _without_ t
 E.g. instead of:
 ```xml
 <properties>
-    <gib.uncommited>true</gib.uncommited>
+    <gib.uncommitted>true</gib.uncommitted>
 </properties>
 ```
 use:
 ```xml
 <configuration>
-    <uncommited>true</uncommited>
+    <uncommitted>true</uncommitted>
 </configuration>
 ```
-Just like for regular plugins, a plugin config property that is set to a _fixed_ value **cannot be changed via the respective system property** (here: `-Dgib.uncommited=false`).
+Just like for regular plugins, a plugin config property that is set to a _fixed_ value **cannot be changed via the respective system property** (here: `-Dgib.uncommitted=false`).
 
 ### gib.help
 
@@ -387,7 +387,7 @@ Since: 3.11.0
 
 ### gib.disableBranchComparison
 
-Disables the comparison between `baseBranch` and `referenceBranch`. This property should be enabled if _only_ uncommitted and/or untracked files shall be detected to only build projects that have been changed since the last commit in the current branch (see `gib.uncommited` and `gib.untracked`).
+Disables the comparison between `baseBranch` and `referenceBranch`. This property should be enabled if _only_ uncommitted and/or untracked files shall be detected to only build projects that have been changed since the last commit in the current branch (see `gib.uncommitted` and `gib.untracked`).
 The following properties are _not_ evaluated when `gib.disableBranchComparison` is enabled:
 - `gib.referenceBranch`
 - `gib.compareToMergeBase`
@@ -440,7 +440,7 @@ Since: 3.9.1
 
 Controls whether or not to the [merge-base](https://git-scm.com/docs/git-merge-base) mechanism to compare the branches.
 
-### gib.uncommited
+### gib.uncommitted
 
 Detects changed files that have not yet been committed. This does **not** include _untracked_ files (see `git status` manual).
 
diff --git a/src/main/java/com/vackosar/gitflowincrementalbuild/boundary/Configuration.java b/src/main/java/com/vackosar/gitflowincrementalbuild/boundary/Configuration.java
index 6a44ca0..4665d88 100644
--- a/src/main/java/com/vackosar/gitflowincrementalbuild/boundary/Configuration.java
+++ b/src/main/java/com/vackosar/gitflowincrementalbuild/boundary/Configuration.java
@@ -48,7 +48,7 @@ public class Configuration {
     public final boolean fetchBaseBranch;
     public final boolean useJschAgentProxy;
     public final boolean compareToMergeBase;
-    public final boolean uncommited;
+    public final boolean uncommitted;
     public final boolean untracked;
     public final Optional<Predicate<String>> excludePathRegex;
     public final Optional<Predicate<String>> includePathRegex;
@@ -89,7 +89,7 @@ public class Configuration {
             fetchBaseBranch = false;
             useJschAgentProxy = false;
             compareToMergeBase = false;
-            uncommited = false;
+            uncommitted = false;
             untracked = false;
             excludePathRegex = null;
             includePathRegex = null;
@@ -130,7 +130,7 @@ public class Configuration {
         fetchBaseBranch = Boolean.valueOf(Property.fetchBaseBranch.getValue(pluginProperties, projectProperties));
         useJschAgentProxy = Boolean.valueOf(Property.useJschAgentProxy.getValue(pluginProperties, projectProperties));
         compareToMergeBase = Boolean.valueOf(Property.compareToMergeBase.getValue(pluginProperties, projectProperties));
-        uncommited = Boolean.valueOf(Property.uncommited.getValue(pluginProperties, projectProperties));
+        uncommitted = Boolean.valueOf(Property.uncommitted.getValue(pluginProperties, projectProperties));
         untracked = Boolean.valueOf(Property.untracked.getValue(pluginProperties, projectProperties));
         excludePathRegex = compileOptionalPatternPredicate(Property.excludePathRegex, pluginProperties, projectProperties);
         includePathRegex = compileOptionalPatternPredicate(Property.includePathRegex, pluginProperties, projectProperties);
diff --git a/src/main/java/com/vackosar/gitflowincrementalbuild/control/DifferentFiles.java b/src/main/java/com/vackosar/gitflowincrementalbuild/control/DifferentFiles.java
index 8344789..7223c64 100644
--- a/src/main/java/com/vackosar/gitflowincrementalbuild/control/DifferentFiles.java
+++ b/src/main/java/com/vackosar/gitflowincrementalbuild/control/DifferentFiles.java
@@ -63,7 +63,7 @@ public class DifferentFiles {
             if (!config.disableBranchComparison) {
                 paths.addAll(worker.getBranchDiff());
             }
-            if (config.uncommited || config.untracked) {
+            if (config.uncommitted || config.untracked) {
                 paths.addAll(worker.getChangesFromStatus());
             }
         } finally {
@@ -204,7 +204,7 @@ public class DifferentFiles {
         private Set<Path> getChangesFromStatus() throws GitAPIException {
             Set<String> changes = new HashSet<>();
             Status status = git.status().call();
-            if (configuration.uncommited) {
+            if (configuration.uncommitted) {
                 changes.addAll(status.getUncommittedChanges());
             }
             if (configuration.untracked) {
diff --git a/src/main/java/com/vackosar/gitflowincrementalbuild/control/Property.java b/src/main/java/com/vackosar/gitflowincrementalbuild/control/Property.java
index 17ed1f1..c66d12c 100644
--- a/src/main/java/com/vackosar/gitflowincrementalbuild/control/Property.java
+++ b/src/main/java/com/vackosar/gitflowincrementalbuild/control/Property.java
@@ -80,7 +80,12 @@ public enum Property {
     /**
      * Detects changed files that have not yet been committed.
      */
-    uncommited("true", "uc", true),
+    uncommitted("true", "uc", true) {
+        @Override
+        public Optional<String> deprecatedName() {
+            return Optional.of("uncommited");
+        }
+    },
     /**
      * Detects files that are not yet tracked by git.
      */
@@ -171,7 +176,7 @@ public enum Property {
         this.nameCandidatesForSystemProperties = Stream.of(prefixedName, prefixedShortName, deprecatedPrefixedName())
                 .filter(Objects::nonNull)
                 .collect(Collectors.collectingAndThen(Collectors.toList(), Collections::unmodifiableList));
-        this.nameCandidatesForPluginProperties = Stream.of(name(), deprecatedName())
+        this.nameCandidatesForPluginProperties = Stream.of(name(), deprecatedName().orElse(null))
                 .filter(Objects::nonNull)
                 .collect(Collectors.collectingAndThen(Collectors.toList(), Collections::unmodifiableList));
         this.nameCandidatesForProjectProperties = Stream.of(prefixedName, deprecatedPrefixedName())
@@ -203,16 +208,16 @@ public enum Property {
     /**
      * @return the deprecated unprefixed name or {@code null} if there is no such deprecated name for this property
      */
-    public String deprecatedName() {
+    public Optional<String> deprecatedName() {
         // might be overridden by specific enum instances
-        return null;
+        return Optional.empty();
     }
 
     /**
      * @return the deprecated prefixed name or {@code null} if there is no such deprecated name for this property
      */
     public final String deprecatedPrefixedName() {
-        return Optional.ofNullable(deprecatedName()).map(PREFIX::concat).orElse(null);
+        return deprecatedName().map(PREFIX::concat).orElse(null);
     }
 
     public ValueWithOriginContext getValueWithOriginContext(Properties pluginProperties, Properties projectProperties) {
@@ -262,7 +267,7 @@ public enum Property {
             return null;
         }
         boolean prefixed = name.startsWith(PREFIX);
-        String deprecatedName = prefixed ? deprecatedPrefixedName() : deprecatedName();
+        String deprecatedName = prefixed ? deprecatedPrefixedName() : deprecatedName().orElse(null);
         if (name.equals(deprecatedName)) {
             LOGGER.warn("'{}' has been renamed to '{}' and the old name will be removed in an upcoming release. Please adjust your configuration!",
                     deprecatedName, prefixed ? prefixedName : name());
diff --git a/src/main/java/com/vackosar/gitflowincrementalbuild/mojo/MojoParametersGeneratingByteBuddyPlugin.java b/src/main/java/com/vackosar/gitflowincrementalbuild/mojo/MojoParametersGeneratingByteBuddyPlugin.java
index 06393d5..0535174 100644
--- a/src/main/java/com/vackosar/gitflowincrementalbuild/mojo/MojoParametersGeneratingByteBuddyPlugin.java
+++ b/src/main/java/com/vackosar/gitflowincrementalbuild/mojo/MojoParametersGeneratingByteBuddyPlugin.java
@@ -3,7 +3,6 @@ package com.vackosar.gitflowincrementalbuild.mojo;
 import java.io.IOException;
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Modifier;
-
 import org.apache.maven.plugins.annotations.InstantiationStrategy;
 import org.apache.maven.plugins.annotations.LifecyclePhase;
 import org.apache.maven.plugins.annotations.Mojo;
@@ -217,7 +216,7 @@ public class MojoParametersGeneratingByteBuddyPlugin implements Plugin {
 
             @Override
             public String alias() {
-                // cannot be used for the short name since in Eclipse this would then be a valid option to select (but only -D is allowed)
+                // Cannot be used for the short name since in Eclipse this would then be a valid option to select (but only -D is allowed).
                 return "";
             }
         };


Test patch:
diff --git a/src/test/java/com/vackosar/gitflowincrementalbuild/BaseRepoTest.java b/src/test/java/com/vackosar/gitflowincrementalbuild/BaseRepoTest.java
index 845d73e..b7c9278 100644
--- a/src/test/java/com/vackosar/gitflowincrementalbuild/BaseRepoTest.java
+++ b/src/test/java/com/vackosar/gitflowincrementalbuild/BaseRepoTest.java
@@ -68,7 +68,7 @@ public abstract class BaseRepoTest {
     }
 
     private void init() {
-        projectProperties.setProperty(Property.uncommited.prefixedName(), "false");
+        projectProperties.setProperty(Property.uncommitted.prefixedName(), "false");
         projectProperties.setProperty(Property.untracked.prefixedName(), "false");
         projectProperties.setProperty(Property.referenceBranch.prefixedName(), "refs/heads/develop");
         projectProperties.setProperty(Property.compareToMergeBase.prefixedName(), "false");
diff --git a/src/test/java/com/vackosar/gitflowincrementalbuild/control/DifferentFilesTest.java b/src/test/java/com/vackosar/gitflowincrementalbuild/control/DifferentFilesTest.java
index 3fd18ea..f7e155b 100644
--- a/src/test/java/com/vackosar/gitflowincrementalbuild/control/DifferentFilesTest.java
+++ b/src/test/java/com/vackosar/gitflowincrementalbuild/control/DifferentFilesTest.java
@@ -53,7 +53,7 @@ public class DifferentFilesTest extends BaseDifferentFilesTest {
     @Test
     public void listWithUncommitted() throws Exception {
         Path modifiedFilePath = modifyTrackedFile(repoPath);
-        projectProperties.setProperty(Property.uncommited.prefixedName(), "true");
+        projectProperties.setProperty(Property.uncommitted.prefixedName(), "true");
 
         assertTrue(invokeUnderTest().contains(modifiedFilePath));
     }
@@ -61,7 +61,7 @@ public class DifferentFilesTest extends BaseDifferentFilesTest {
     @Test
     public void listWithUncommitted_disabled() throws Exception {
         Path modifiedFilePath = modifyTrackedFile(repoPath);
-        projectProperties.setProperty(Property.uncommited.prefixedName(), "false");
+        projectProperties.setProperty(Property.uncommitted.prefixedName(), "false");
 
         assertFalse(invokeUnderTest().contains(modifiedFilePath));
     }
@@ -69,7 +69,7 @@ public class DifferentFilesTest extends BaseDifferentFilesTest {
     @Test
     public void listWithUncommitted_excluded() throws Exception {
         Path modifiedFilePath = modifyTrackedFile(repoPath);
-        projectProperties.setProperty(Property.uncommited.prefixedName(), "true");
+        projectProperties.setProperty(Property.uncommitted.prefixedName(), "true");
         projectProperties.setProperty(Property.excludePathRegex.prefixedName(), Pattern.quote(repoPath.relativize(modifiedFilePath).toString()));
 
         assertFalse(invokeUnderTest().contains(modifiedFilePath));
diff --git a/src/test/java/com/vackosar/gitflowincrementalbuild/mojo/FakeMojoTest.java b/src/test/java/com/vackosar/gitflowincrementalbuild/mojo/FakeMojoTest.java
index 48269af..40b12b9 100644
--- a/src/test/java/com/vackosar/gitflowincrementalbuild/mojo/FakeMojoTest.java
+++ b/src/test/java/com/vackosar/gitflowincrementalbuild/mojo/FakeMojoTest.java
@@ -202,8 +202,8 @@ public class FakeMojoTest implements WithAssertions {
 
     private void check(String asTextFormat, ImmutablePair<List<String>, List<String>> nodeTexts, Consumer<ProxyableListAssert<String>> assertionConsumer) {
         SoftAssertions softly = new SoftAssertions();
-        assertionConsumer.accept(softly.assertThat(nodeTexts.left) .as(asTextFormat, "plugin.xml"));
-        assertionConsumer.accept(softly.assertThat(nodeTexts.right) .as(asTextFormat, "plugin-help.xml"));
+        assertionConsumer.accept(softly.assertThat(nodeTexts.left).as(asTextFormat, "plugin.xml"));
+        assertionConsumer.accept(softly.assertThat(nodeTexts.right).as(asTextFormat, "plugin-help.xml"));
         softly.assertAll();
     }
 }