You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2021/05/28 08:54:47 UTC
[GitHub] [james-project] chibenwa opened a new pull request #461: JAMES-2886 Fix configuration parsing for Guice extensions
chibenwa opened a new pull request #461:
URL: https://github.com/apache/james-project/pull/461
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] jeantil commented on a change in pull request #461: JAMES-2886 Fix configuration parsing for Guice extensions
Posted by GitBox <gi...@apache.org>.
jeantil commented on a change in pull request #461:
URL: https://github.com/apache/james-project/pull/461#discussion_r641567950
##########
File path: server/container/guice/guice-utils/src/main/java/org/apache/james/utils/ExtensionConfiguration.java
##########
@@ -19,24 +19,25 @@
package org.apache.james.utils;
-import java.util.Arrays;
import java.util.List;
-import java.util.Optional;
import org.apache.commons.configuration2.Configuration;
import com.github.steveash.guavate.Guavate;
+import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
public class ExtensionConfiguration {
public static final ExtensionConfiguration DEFAULT = new ExtensionConfiguration(ImmutableList.of());
public static ExtensionConfiguration from(Configuration configuration) {
- List<String> list = Optional.ofNullable(configuration.getStringArray("guice.extension.module"))
- .map(Arrays::asList)
- .orElse(ImmutableList.of());
+ String rawString = configuration.getString("guice.extension.module", "");
- return new ExtensionConfiguration(list.stream()
+ return new ExtensionConfiguration(Splitter.on(",")
+ .omitEmptyStrings()
+ .trimResults()
Review comment:
I feel that the splitter setup gets in the way of the actual parsing logic.
The first 3 lines are type Splitter, then the flow changes to stream processing with the actual input value only appearing on line 4
extracting to a constant would change the code to
```
SPLITTER.splitToList(rawString)
.stream()
.map(ClassName::new)
.collect(Guavate.toImmutableList())
```
An alternative could be extracting to a private method that event takes care of the list<->stream ceremony
```
splitOnCommas(rawString)
.map(ClassName::new)
.collect(Guavate.toImmutableList())
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] jeantil commented on a change in pull request #461: JAMES-2886 Fix configuration parsing for Guice extensions
Posted by GitBox <gi...@apache.org>.
jeantil commented on a change in pull request #461:
URL: https://github.com/apache/james-project/pull/461#discussion_r641519732
##########
File path: server/container/guice/guice-utils/src/main/java/org/apache/james/utils/ExtensionConfiguration.java
##########
@@ -19,24 +19,25 @@
package org.apache.james.utils;
-import java.util.Arrays;
import java.util.List;
-import java.util.Optional;
import org.apache.commons.configuration2.Configuration;
import com.github.steveash.guavate.Guavate;
+import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
public class ExtensionConfiguration {
public static final ExtensionConfiguration DEFAULT = new ExtensionConfiguration(ImmutableList.of());
public static ExtensionConfiguration from(Configuration configuration) {
- List<String> list = Optional.ofNullable(configuration.getStringArray("guice.extension.module"))
- .map(Arrays::asList)
- .orElse(ImmutableList.of());
+ String rawString = configuration.getString("guice.extension.module", "");
- return new ExtensionConfiguration(list.stream()
+ return new ExtensionConfiguration(Splitter.on(",")
+ .omitEmptyStrings()
+ .trimResults()
Review comment:
maybe extract
```
Splitter.on(",")
.omitEmptyStrings()
.trimResults()
```
to a local constant ? I don't think we need to build a new instance for every call (not that it matters much since this should only run once per process)
##########
File path: server/container/guice/protocols/webadmin/src/main/java/org/apache/james/modules/server/WebAdminServerModule.java
##########
@@ -138,6 +138,16 @@ public WebAdminConfiguration provideWebAdminConfiguration(FileSystem fileSystem,
}
}
+ @VisibleForTesting
+ ImmutableList<String> additionalRoutes(Configuration configurationFile) {
+ String rawString = configurationFile.getString("extensions.routes", "");
+
+ return ImmutableList.copyOf(Splitter.on(',')
Review comment:
same as above: extract to local constant (note that I don't think mutualizing the two would be beneficial)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #461: JAMES-2886 Fix configuration parsing for Guice extensions
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #461:
URL: https://github.com/apache/james-project/pull/461#discussion_r641661721
##########
File path: server/container/guice/guice-utils/src/main/java/org/apache/james/utils/ExtensionConfiguration.java
##########
@@ -19,24 +19,25 @@
package org.apache.james.utils;
-import java.util.Arrays;
import java.util.List;
-import java.util.Optional;
import org.apache.commons.configuration2.Configuration;
import com.github.steveash.guavate.Guavate;
+import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
public class ExtensionConfiguration {
public static final ExtensionConfiguration DEFAULT = new ExtensionConfiguration(ImmutableList.of());
public static ExtensionConfiguration from(Configuration configuration) {
- List<String> list = Optional.ofNullable(configuration.getStringArray("guice.extension.module"))
- .map(Arrays::asList)
- .orElse(ImmutableList.of());
+ String rawString = configuration.getString("guice.extension.module", "");
- return new ExtensionConfiguration(list.stream()
+ return new ExtensionConfiguration(Splitter.on(",")
+ .omitEmptyStrings()
+ .trimResults()
Review comment:
I had a look, Joiner and Splitter are thread safe, thus can be safely shared as constants
The point is we never did it in the source code. I think it could be nice to conduct a global refactoring regarding this.
I guess the best way to move is thus fix these usage here and open another PR for doing this refctoring.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa merged pull request #461: JAMES-2886 Fix configuration parsing for Guice extensions
Posted by GitBox <gi...@apache.org>.
chibenwa merged pull request #461:
URL: https://github.com/apache/james-project/pull/461
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #461: JAMES-2886 Fix configuration parsing for Guice extensions
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #461:
URL: https://github.com/apache/james-project/pull/461#issuecomment-851113832
(Forced pushed to squash them)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #461: JAMES-2886 Fix configuration parsing for Guice extensions
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #461:
URL: https://github.com/apache/james-project/pull/461#discussion_r641524791
##########
File path: server/container/guice/guice-utils/src/main/java/org/apache/james/utils/ExtensionConfiguration.java
##########
@@ -19,24 +19,25 @@
package org.apache.james.utils;
-import java.util.Arrays;
import java.util.List;
-import java.util.Optional;
import org.apache.commons.configuration2.Configuration;
import com.github.steveash.guavate.Guavate;
+import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
public class ExtensionConfiguration {
public static final ExtensionConfiguration DEFAULT = new ExtensionConfiguration(ImmutableList.of());
public static ExtensionConfiguration from(Configuration configuration) {
- List<String> list = Optional.ofNullable(configuration.getStringArray("guice.extension.module"))
- .map(Arrays::asList)
- .orElse(ImmutableList.of());
+ String rawString = configuration.getString("guice.extension.module", "");
- return new ExtensionConfiguration(list.stream()
+ return new ExtensionConfiguration(Splitter.on(",")
+ .omitEmptyStrings()
+ .trimResults()
Review comment:
This code path is only executed once.
Do we care? Sounds like early optimisation to me...
##########
File path: server/container/guice/protocols/webadmin/src/main/java/org/apache/james/modules/server/WebAdminServerModule.java
##########
@@ -138,6 +138,16 @@ public WebAdminConfiguration provideWebAdminConfiguration(FileSystem fileSystem,
}
}
+ @VisibleForTesting
+ ImmutableList<String> additionalRoutes(Configuration configurationFile) {
+ String rawString = configurationFile.getString("extensions.routes", "");
+
+ return ImmutableList.copyOf(Splitter.on(',')
Review comment:
This code path is only executed once.
Do we care? Sounds like early optimisation to me...
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org