[GitHub] mikewalch commented on a change in pull request #902: fixes #894 merge SimpleConfiguration Objects
mikewalch commented on a change in pull request #902: fixes #894 merge
SimpleConfiguration Objects
URL: https://github.com/apache/fluo/pull/902#discussion_r132228853
##
File path:
modules/api/src/main/java/org/apache/fluo/api/config/SimpleConfiguration.java
##
@@ -250,6 +250,40 @@ public SimpleConfiguration subset(String prefix) {
return new SimpleConfiguration(internalConfig.subset(prefix));
}
+ /**
+ * @param fallback SimpleConfiguration to join together
+ * @return a new simple configuration that contains all of the current
properties from this plus
+ * the properties from fallback that are not present in this.
+ *
+ * @since 1.2.0
+ */
+ public SimpleConfiguration orElse(SimpleConfiguration fallback) {
+for (Map.Entry entry : fallback.toMap().entrySet()) {
+ if (!this.containsKey(entry.getKey())) {
+this.setProperty(entry.getKey(), entry.getValue());
+ }
+}
+return this;
+ }
+
+ @Override
+ public int hashCode() {
+return Objects.hashCode(this.toString());
+ }
+
+ @Override
+ public boolean equals(Object o) {
+if (o == this) {
+ return true;
+}
+
+if (o instanceof SimpleConfiguration) {
+ SimpleConfiguration sc = (SimpleConfiguration) o;
+ return this.toString().equals(sc.toString());
Review comment:
I am not sure about checking for equality by comparing toString(). It
relies on toString() method to be very consistent. Can properties be printed in
a different order? If so, comparing two identical configuration objects could
return false. For this comparison, you could convert each simple configuration
to map, verify that the maps contain same number of entries, and verify that
all properties & values in on sc matches the other.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mikewalch commented on a change in pull request #902: fixes #894 merge SimpleConfiguration Objects
mikewalch commented on a change in pull request #902: fixes #894 merge
SimpleConfiguration Objects
URL: https://github.com/apache/fluo/pull/902#discussion_r131693457
##
File path:
modules/api/src/main/java/org/apache/fluo/api/config/SimpleConfiguration.java
##
@@ -250,6 +277,22 @@ public SimpleConfiguration subset(String prefix) {
return new SimpleConfiguration(internalConfig.subset(prefix));
}
+ /**
+ * @param SimpleConfigs SimpleConfiguration's to be merged
+ * @return SimpleConfiguration
+ *
+ * @since 1.2.0
+ */
+ public static SimpleConfiguration merge(SimpleConfiguration...
simpleConfigs) {
+SimpleConfiguration mrg = new SimpleConfiguration();
+for (SimpleConfiguration sc : simpleConfigs) {
+ for (Map.Entry entry : sc.toMap().entrySet()) {
+mrg.addProperty(entry.getKey(), entry.getValue());
Review comment:
I might be missing something (maybe @keith-turner can weigh in too) but I
think the goal of this ticket is to merge the files so each the final
configuration contains a union of all properties. If properties are in multiple
files, only one value should be chosen (so setProperty should be used to
merge). The merge method just needs to document which file will take
precedent. If addProperty() is used, a list of values will be returned to the
user when they retrieve a property that was in multiple files.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mikewalch commented on a change in pull request #902: fixes #894 merge SimpleConfiguration Objects
mikewalch commented on a change in pull request #902: fixes #894 merge
SimpleConfiguration Objects
URL: https://github.com/apache/fluo/pull/902#discussion_r131681570
##
File path:
modules/api/src/main/java/org/apache/fluo/api/config/SimpleConfiguration.java
##
@@ -250,6 +277,22 @@ public SimpleConfiguration subset(String prefix) {
return new SimpleConfiguration(internalConfig.subset(prefix));
}
+ /**
+ * @param SimpleConfigs SimpleConfiguration's to be merged
+ * @return SimpleConfiguration
+ *
+ * @since 1.2.0
+ */
+ public static SimpleConfiguration merge(SimpleConfiguration...
simpleConfigs) {
+SimpleConfiguration mrg = new SimpleConfiguration();
+for (SimpleConfiguration sc : simpleConfigs) {
+ for (Map.Entry entry : sc.toMap().entrySet()) {
+mrg.addProperty(entry.getKey(), entry.getValue());
Review comment:
Should setProperty be used here instead? It looks like
[addProperty()](http://commons.apache.org/proper/commons-configuration/javadocs/v1.10/apidocs/org/apache/commons/configuration/Configuration.html#addProperty(java.lang.String,%20java.lang.Object))
will create a list of values if a property is already set.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mikewalch commented on a change in pull request #902: fixes #894 merge SimpleConfiguration Objects
mikewalch commented on a change in pull request #902: fixes #894 merge
SimpleConfiguration Objects
URL: https://github.com/apache/fluo/pull/902#discussion_r131682349
##
File path:
modules/api/src/main/java/org/apache/fluo/api/config/SimpleConfiguration.java
##
@@ -250,6 +277,22 @@ public SimpleConfiguration subset(String prefix) {
return new SimpleConfiguration(internalConfig.subset(prefix));
}
+ /**
+ * @param SimpleConfigs SimpleConfiguration's to be merged
+ * @return SimpleConfiguration
Review comment:
Should document how properties are merged if they are set in multiple
SimpleConfigurations. Also does first configuration parameter listed take
precedent or does last?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
