Add better support for multiple top-level values#2783
Add better support for multiple top-level values#2783Marcono1234 wants to merge 5 commits intogoogle:mainfrom
Conversation
| public final void setMultiTopLevelValuesAllowed(boolean enabled) { | ||
| this.multiTopLevelValuesEnabled = enabled; | ||
| } | ||
|
|
||
| /** | ||
| * Returns whether multiple top-level values are allowed. | ||
| * | ||
| * @see #setMultiTopLevelValuesAllowed(boolean) | ||
| * @since $next-version$ | ||
| */ | ||
| public final boolean isMultiTopLevelValuesAllowed() { | ||
| return multiTopLevelValuesEnabled; | ||
| } |
There was a problem hiding this comment.
I am not completely happy about these method names setMultiTopLevelValuesAllowed and isMultiTopLevelValuesAllowed yet. Any feedback and suggestions are welcome.
There was a problem hiding this comment.
What about setAllowingMultipleValues and isAllowingMultipleValues? "Multiple values" isn't quite as precise as "multi top-level values", but the latter is kind of verbose. I'm proposing "allowing X" rather than "X allowed" to avoid the grammatical issue with "is top-level values allowed".
There was a problem hiding this comment.
Ok, have renamed the methods. I have left the corresponding test methods unchanged (enabled/disabled) to make it more obvious that they are about explicitly enabling / disabling this.
|
An alternative would be to always enable support for multiple top-level values, regardless of strictness, and then only have Though since there are multiple formats / standards for encoding multiple top-level values, maybe this is not desired? And could cause confusion when users accidentally write or read multiple top-level values? (though a bit unlikely?) |
eamonnmcmanus
left a comment
eamonnmcmanus
left a comment
There was a problem hiding this comment.
If you are on for reviving this, I have run across some use cases for it and would be interested in following through, ideally before the next release.
| public final void setMultiTopLevelValuesAllowed(boolean enabled) { | ||
| this.multiTopLevelValuesEnabled = enabled; | ||
| } | ||
|
|
||
| /** | ||
| * Returns whether multiple top-level values are allowed. | ||
| * | ||
| * @see #setMultiTopLevelValuesAllowed(boolean) | ||
| * @since $next-version$ | ||
| */ | ||
| public final boolean isMultiTopLevelValuesAllowed() { | ||
| return multiTopLevelValuesEnabled; | ||
| } |
There was a problem hiding this comment.
What about setAllowingMultipleValues and isAllowingMultipleValues? "Multiple values" isn't quite as precise as "multi top-level values", but the latter is kind of verbose. I'm proposing "allowing X" rather than "X allowed" to avoid the grammatical issue with "is top-level values allowed".
| * @see #getTopLevelSeparator() | ||
| * @since $next-version$ | ||
| */ | ||
| public final void setTopLevelSeparator(String separator) { |
There was a problem hiding this comment.
Not sure if we should also support an boolean addTrailing parameter, so that the JsonWriter emits the separator also after the last element.
It seems JSON Lines recommends adding a trailing line break, though does not require it. Not sure about other multi-value JSON standards.
2 participants
Purpose
Add better support for reading and writing multiple top-level values
(the user can now customize the separator between top-level values, so that is preferred over making
JsonReadertell more ambiguous concatenated top-level value apart)Description
Currently reading and writing multiple top-level JSON values is coupled with lenient mode. This might be undesired for users who normally want to use strict mode.
Having multiple top-level values is a common use case when streaming JSON values, see for example JSON Lines. And another problem is that
JsonWritercurrently concatenates multiple JSON values without any separating whitespace, and without any way to configure it.Other JSON libraries seem to have dedicated classes or configuration for multiple top-level value support, e.g. Jackson's
MappingIteratorandSequenceWriterand kotlinx-serialization'sDecodeSequenceMode.WHITESPACE_SEPARATED.However, because Gson's
JsonReaderandJsonWriteralready had (limited) functionality for multiple top-level values, I added new API to those classes instead of adding completely new classes.Exposing this also through
Gson/GsonBuildermight not be needed since the methods of theGsonclass are mainly for reading and writing a single value.Checklist
This is automatically checked by
mvn verify, but can also be checked on its own usingmvn spotless:check.Style violations can be fixed using
mvn spotless:apply; this can be done in a separate commit to verify that it did not cause undesired changes.null@since $next-version$(
$next-version$is a special placeholder which is automatically replaced during release)TestCase)mvn clean verify javadoc:jarpasses without errors