-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add better support for multiple top-level values #2783
base: main
Are you sure you want to change the base?
Add better support for multiple top-level values #2783
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not completely happy about these method names setMultiTopLevelValuesAllowed
and isMultiTopLevelValuesAllowed
yet. Any feedback and suggestions are welcome.
} | ||
pos--; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this pos--;
outside the if-else since the if { ... }
performs a return
.
} | ||
// fall-through |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this "fall-through" was previously redundant: The next case is EMPTY_DOCUMENT
which replaces the top of the stack with NONEMPTY_DOCUMENT
, but here the top is already NONEMPTY_DOCUMENT
.
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?) |
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
JsonReader
tell 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
JsonWriter
currently 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
MappingIterator
andSequenceWriter
and kotlinx-serialization'sDecodeSequenceMode.WHITESPACE_SEPARATED
.However, because Gson's
JsonReader
andJsonWriter
already 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
/GsonBuilder
might not be needed since the methods of theGson
class 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:jar
passes without errors