Skip to content
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

GH-43 Adds support for serialization of amount number as string #45

Closed
wants to merge 4 commits into from

Conversation

AlexanderYastrebov
Copy link
Member

Fixes #43

Copy link
Collaborator

@whiskeysierra whiskeysierra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs some readme change.

@@ -50,11 +60,15 @@ public void serialize(final MonetaryAmount value, final JsonGenerator generator,
generator.writeEndObject();
}

static boolean numberAsStringDefault() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

@@ -16,6 +16,7 @@

private final MonetaryAmountFormatFactory factory;
private final FieldNames names;
private final boolean numberAsString;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using a boolean, can we use our own implementation of Jackson's Feature types?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially implemented features the same way as jackson does, but then discarded it since I hope there will not be that many toggles at all

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still prefer a Feature enum.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean own or Jackson's? With Jackson's we have to fail if someone requests other feature beside WRITE_NUMBERS_AS_STRINGS

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could implement ConfigFeature ourselves, but I think what would be a better fit would be an enum:

public enum Style {
    DECIMAL, STRING
}

@whiskeysierra
Copy link
Collaborator

I just found this: http://fasterxml.github.io/jackson-core/javadoc/2.8/com/fasterxml/jackson/core/JsonGenerator.Feature.html#WRITE_NUMBERS_AS_STRINGS

Feature that forces all Java numbers to be written as JSON strings. Default state is 'false', meaning that Java numbers are to be serialized using basic numeric serialization (as JSON numbers, integral or floating point). If enabled, all such numeric values are instead written out as JSON Strings.
One use case is to avoid problems with Javascript limitations: since Javascript standard specifies that all number handling should be done using 64-bit IEEE 754 floating point values, result being that some 64-bit integer values can not be accurately represent (as mantissa is only 51 bit wide).

Feature is disabled by default.

@AlexanderYastrebov
Copy link
Member Author

I just found this

I have seen it.
It changes behavior for all numbers and we want only number-as-string within monetary amount.

@AlexanderYastrebov
Copy link
Member Author

@whiskeysierra RFC

@whiskeysierra
Copy link
Collaborator

It changes behavior for all numbers and we want only number-as-string within monetary amount.

True, but client at least have an option right now already.

@AlexanderYastrebov
Copy link
Member Author

@whiskeysierra I dont get your point, could you explain?

@whiskeysierra
Copy link
Collaborator

If somebody want to use strings instead of decimal number he/she can already use Jackson's built-in support for that (they might be ok with all numbers being strings).

@AlexanderYastrebov
Copy link
Member Author

Ah, so you want default value to be inherited from that configured jackson feature?

@whiskeysierra
Copy link
Collaborator

Once WRITE_NUMBERS_AS_STRINGS is enabled it will overrule anything in this PR.

@AlexanderYastrebov
Copy link
Member Author

AlexanderYastrebov commented May 5, 2017

Once WRITE_NUMBERS_AS_STRINGS is enabled it will overrule anything in this PR.

And this is desired, isnt it?

@AlexanderYastrebov
Copy link
Member Author

Or do you want to say this feature is not needed at all?
The value is that it allows to serialize as strings only monetary amounts and leave other numerals intact

@whiskeysierra
Copy link
Collaborator

Or do you want to say this feature is not needed at all?

Well, I'm ambivalent. On the one hand I'd say it's not needed since Jackson kind of already supports it.

The value is that it allows to serialize as strings only monetary amounts and leave other numerals intact

On the other hand I see the value in that. That feature however could be understood in a more general concept: It would be nice to enable/disable certain features for certain types/subtypes/object graphs/...

It does not apply to monetary amounts alone, but could be useful to others as well. No idea how to tackle this, though.

@AlexanderYastrebov
Copy link
Member Author

After some consideration we decided to implement it as an pluggable interface instead of feature toggle.
I will update PR.

@AlexanderYastrebov
Copy link
Member Author

Closing in favor of #48

@whiskeysierra whiskeysierra deleted the GH-43-serialize-amount-as-string branch August 14, 2017 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants