-
Notifications
You must be signed in to change notification settings - Fork 16
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
Json primitive map keys #319
base: master
Are you sure you want to change the base?
Changes from all commits
873d9ac
f4aff2a
e5eec78
07673aa
8c78d53
12aa6a9
419e565
9e46b0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
name: Scala CI | ||
|
||
on: | ||
[push, pull_request] | ||
|
||
jobs: | ||
build: | ||
|
||
runs-on: ubuntu-latest | ||
|
||
steps: | ||
- uses: actions/checkout@v2 | ||
- run: git fetch --prune --unshallow | ||
- name: Set up JDK 1.8 | ||
uses: actions/setup-java@v1 | ||
with: | ||
java-version: 1.8 | ||
- name: Run tests | ||
run: sbt test |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,7 +74,9 @@ final private[borer] class JsonRenderer(var out: Output) extends Renderer { | |
def onLong(value: Long): Unit = | ||
if (isNotMapKey) { | ||
out = count(writeLong(sep(out), value)) | ||
} else failCannotBeMapKey("integer values") | ||
} else { | ||
out = count(writeLong(sep(out).writeAsByte('"'), value).writeAsByte('"')) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's move that into it's own method, so that it doesn't get inlined when the JVM decides to inline the Also, what about booleans, ints, overlongs, floats, doubles and number strings? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I will add that. As I wrote, I first wanted to be sure the style I am implementing it is OK with you. |
||
} | ||
|
||
def onOverLong(negative: Boolean, value: Long): Unit = | ||
if (isNotMapKey) { | ||
|
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.
Hmm, I don't think it's a good idea to generally attempt to parse a
String
element when the user want's to read a number. If the user wants to read ashort
we shouldn't accept aString
.Remember that borer is first and foremost a CBOR serialization library and supports JSON only as a subset of what CBOR offers.
If we want to enable this "read-simple-types-from-strings" feature we should at least put it behind a configuration flag. Also, we'd have to provide rock-solid error handling since we are now parsing in the
Reader
itself. And: We'd have to make sure that this additional code doesn't end up hurting performance, e.g. by increasing the method size beyond some inlining limit.Luckily we are outside of the main hot path here and can easily move all the code in the
else
branch out into a separate method, where it doesn't mess with JVM inlining scopes.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 would prefer enabling this for map keys only, but as I wrote, I have no idea how to check if I am parsing a map key or not. Do you see some way for that?
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.
Is there any parsing with error handling ready for individual numeric types I could use here, or should I use
readLongFromString
as I do now and check for the range?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.
Hmm... what's your reasoning behind only allowing this auto-conversion on map keys and not other elements as well? What's so special about map keys? To me they are are simply data elements like every other one as well.
The
Reader.Config
interface already has two other config flags calledreadIntegersAlsoAsFloatingPoint
andreadDoubleAlsoAsFloat
. We can simply addreadStringsAlsoAsPrimitives
, which would enable this auto-conversion everywhere (but the default should befalse
).(And while we are at it, breaking binary compatibility, we can also rename
readDoubleAlsoAsFloat
toreadDoublesAlsoAsFloat
for better consistency.)I would simply let
java.lang.Integer.parseInteger
,java.lang.Short.parseShort
,java.lang.Float.parseFloat
, etc. do the job and wrap all exceptions in aBorer.InvalidInputData
.When reading booleans from a String I would also allow for "off" and "no" to be read as
false
and "on" and "yes" to be read astrue
, in addition to "false" and "true".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 thinking about it as auto-conversion, more like map key encoding / decoding - which is a topic of this PR. I have implemented the encoding, that I think is easy and straightforward. I need some decoding as well. A universal auto-conversion is a possible way, but as it achieves more than desired, it raises the need of config flags.
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 don't quite get the point of your last comment.
What is it that you are suggesting?
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 just trying to explain why I wanted to limit the string parsing to map key. I have no idea how to achieve that, therefore I am unable to suggest something. I leave the decision to you - if you think a broader implementation accepting strings as primitives everywhere is good with you, I can proceed.
There is only one minor point I do not like about this: it will be possible to encode a map to JSON without any flags, the numeric keys will be encoded as string, but when one wants to decode the result of such encoding, it will be necessary to use the config allowing
readStringsAlsoAsPrimitives
. If you think this is fine, I can live with that.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.
The
Reader
abstraction has no way of knowing what the role of the next data element is. It doesn't have to know, because in CBOR all data element types can appear in all "positions". It's only JSON that limits certain positions (map keys) to certain types (Strings).As such there is no readily available way to restrict the "primitives-from-string" auto-conversion to map keys at the reader level and I wouldn't like to change the design or add additional layers/complexity just for this (minor, IMHO) feature.
So, I'm afraid it's going to be an all-or-nothing decision for or against the
readStringsAlsoAsPrimitives
functionality.Yes, I agree. This isn't perfect but acceptable to me. borer offers configuration options at the encoding or decoding level, not overarching, across everything. A certain amount of asymmetry is ok, I think.
We already have
readIntegersAlsoAsFloatingPoint
andreadDoubleAlsoAsFloat
, which are also somewaht asymmetric.