[GitHub] [james-project] jeantil commented on a change in pull request #484: JAMES-2886 Fix collection handling for PropertiesProvider

[GitBox <gi...@apache.org>]

jeantil commented on a change in pull request #484:
URL: https://github.com/apache/james-project/pull/484#discussion_r647322652



##########
File path: server/container/guice/configuration/src/main/java/org/apache/james/utils/PropertiesProvider.java
##########
@@ -43,11 +43,20 @@
 import com.google.common.base.Strings;
 
 public class PropertiesProvider {
-
     private static final Logger LOGGER = LoggerFactory.getLogger("org.apache.james.CONFIGURATION");
     private static final char COMMA = ',';
     private static final String COMMA_STRING = ",";
 
+    public static Configuration getConfiguration(File propertiesFile) throws ConfigurationException {

Review comment:
       :thinking: move from private instance method to public static ... ? that's a huge scoping increase ... 
   
   I'm not saying it's wrong but it feels really weird and it seems to be done exclusively for tests, isn't that a bit extreme ?

##########
File path: server/container/guice/configuration/src/main/java/org/apache/james/utils/DelegatedPropertiesConfiguration.java
##########
@@ -372,6 +377,7 @@ public void unlock(LockMode mode) {
 
     private Stream<String> splitAndStripDoubleQuotes(String value) {
         return Optional.ofNullable(value)
+            .filter(s -> !StringUtils.isAllBlank(s))

Review comment:
       shouldn't filtering come after
   ```
   .map(String::trim) ?
   ```
   instead ? to avoid leaving empty values such as `" "` flow through and be converted to `""` ?




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