Skip to content

Conversation

lukaszlenart
Copy link
Member

@lukaszlenart lukaszlenart commented May 12, 2017

This PR introduces Locale aware conversion of BigDecimal and Double and Double

WW-3171
WW-3650
WW-4581

@lukaszlenart lukaszlenart changed the title WW-3171 WW-3357 WW-3650 WW-4581: Locale aware converters WW-3171 WW-3650 WW-4581: Locale aware converters May 12, 2017
@aleksandr-m
Copy link
Contributor

aleksandr-m commented May 15, 2017

Will this break apps with locales which are using different decimal separator? E.g. in db 123.456 and app with locale which uses , as a decimal separator.

@lukaszlenart
Copy link
Member Author

It's all about String -> BigDecimal/Double -> String conversion, so if you have a Double in DB represented as Double it shouldn't be a problem. This solves posting numbers with proper locale and decimal separator and displaying those other way the same.

See this example https://github.com/apache/struts-examples/blob/master/type-conversion/src/main/java/org/apache/struts/example/NumberAction.java#L13-L15

@cnenning
Copy link
Member

In our apps we have often demand for locale aware values. But more often we need dates, not doubles. Mostly we do this by calling java methods in JSPs. But having converts which do this out of the box would be great.

Will this break apps with locales which are using different decimal separator?

Yes, for some apps that would be a breaking change. But IMHO most apps use number formats which their users expect (-> locale aware number format). So for most it would be an improvement and they could remove custom code. But still, some apps might be broken.

I always find it hard to get java NumberFormat and ParsePositon right. Not sure if there are subtle bugs in this PR. But there are lots of tests so hopefully all corner cases are covered 😉

@lukaszlenart
Copy link
Member Author

ParsePosition is basically used to check if the whole value was used, e.g. 1234abc can be normally parsed into 1234 but comparing the ParsePosition with length of the value you can catch such problems.

Also please notice that I have to change just two old tests because of how double was represented - I hope that this change is as less intrusive as possible :)

@cnenning can you elaborate a bit more about parsing dates? I thought this is already supported.

@cnenning
Copy link
Member

can you elaborate a bit more about parsing dates? I thought this is already supported.

Now that you mention it, I see there is a locale aware DateConverter. Cannot remember why we rolled our own. Might be it was not using the format (SHORT, MEDIUM, LONG) we wanted. wdyt about making that configurable?

@lukaszlenart
Copy link
Member Author

Sure thing, please register an issue

@aleksandr-m
Copy link
Contributor

What about Float and float? I remember writing custom converter for it.

@lukaszlenart
Copy link
Member Author

@aleksandr-m good point, done

@lukaszlenart
Copy link
Member Author

Any other objections? I would start with what we have here and improve

@cnenning
Copy link
Member

👍 for merging

@asfgit asfgit merged commit 51afa63 into apache:master May 18, 2017
@lukaszlenart lukaszlenart deleted the locale-aware-converters branch May 18, 2017 13:30
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.

4 participants