johnzon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [johnzon] rmannibucau commented on a change in pull request #67: [JOHNZON-319] Map all properties supported by MapperBuilder in JohnzonBuilder
Date Wed, 29 Jul 2020 07:18:21 GMT

rmannibucau commented on a change in pull request #67:
URL: https://github.com/apache/johnzon/pull/67#discussion_r462057515



##########
File path: johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JohnzonBuilder.java
##########
@@ -131,9 +131,12 @@ public Jsonb build() {
                     return it;
                 }).orElse(false);
 
-        if (config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).orElse(false))
{
-            builder.setPretty(true);
-        }
+        config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).ifPresent(builder::setPretty);
+        config.getProperty(AbstractJsonFactory.BUFFER_STRATEGY).map(String::valueOf).ifPresent(builder::setBufferStrategy);
+        config.getProperty(JsonParserFactoryImpl.BUFFER_LENGTH).map(Integer.class::cast).ifPresent(builder::setBufferSize);
+        config.getProperty(JsonParserFactoryImpl.MAX_STRING_LENGTH).map(Integer.class::cast).ifPresent(builder::setMaxSize);
+        config.getProperty(JsonParserFactoryImpl.SUPPORTS_COMMENTS).map(Boolean.class::cast).ifPresent(builder::setSupportsComments);
+        config.getProperty(JsonParserFactoryImpl.ENCODING).map(String::valueOf).ifPresent(builder::setEncoding);

Review comment:
       Hmm, any PR must not use as jsonb properties some jsonp properties by design or it
should use a filter like + mapping algorithm (prefix or so) to create a kind of passthrough
mode for properties. Aliases are current way to code it, subnamespace would work too. Jsonb
module must also not include johnzon-core imports. in these regards this PR is not mergeable
since it creates a naming inconsistency and breaks module design, this is why I mentionned
it.
   
   Thinking out loud i wouldnt map all properties for jsonp but a single "properties" (map)
one and wire it in all jsonb config consumers (jaxrs providers for ex).




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



Mime
View raw message