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

WW-3171 WW-3650 WW-4581: Locale aware converters #138

Merged
merged 6 commits into from May 18, 2017

Conversation

Projects
None yet
4 participants
@lukaszlenart
Contributor

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 from WW-3171 WW-3357 WW-3650 WW-4581: Locale aware converters to WW-3171 WW-3650 WW-4581: Locale aware converters May 12, 2017

@aleksandr-m

This comment has been minimized.

Show comment
Hide comment
@aleksandr-m

aleksandr-m May 15, 2017

Contributor

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.

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

This comment has been minimized.

Show comment
Hide comment
@lukaszlenart

lukaszlenart May 15, 2017

Contributor

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

Contributor

lukaszlenart commented May 15, 2017

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

This comment has been minimized.

Show comment
Hide comment
@cnenning

cnenning May 16, 2017

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 😉

Member

cnenning commented May 16, 2017

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

This comment has been minimized.

Show comment
Hide comment
@lukaszlenart

lukaszlenart May 16, 2017

Contributor

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.

Contributor

lukaszlenart commented May 16, 2017

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

This comment has been minimized.

Show comment
Hide comment
@cnenning

cnenning May 16, 2017

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?

Member

cnenning commented May 16, 2017

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

This comment has been minimized.

Show comment
Hide comment
@lukaszlenart

lukaszlenart May 16, 2017

Contributor

Sure thing, please register an issue

Contributor

lukaszlenart commented May 16, 2017

Sure thing, please register an issue

@aleksandr-m

This comment has been minimized.

Show comment
Hide comment
@aleksandr-m

aleksandr-m May 16, 2017

Contributor

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

Contributor

aleksandr-m commented May 16, 2017

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

@lukaszlenart

This comment has been minimized.

Show comment
Hide comment
@lukaszlenart

lukaszlenart May 17, 2017

Contributor

@aleksandr-m good point, done

Contributor

lukaszlenart commented May 17, 2017

@aleksandr-m good point, done

@lukaszlenart

This comment has been minimized.

Show comment
Hide comment
@lukaszlenart

lukaszlenart May 18, 2017

Contributor

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

Contributor

lukaszlenart commented May 18, 2017

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

@cnenning

This comment has been minimized.

Show comment
Hide comment
@cnenning

cnenning May 18, 2017

Member

👍 for merging

Member

cnenning commented May 18, 2017

👍 for merging

@asfgit asfgit merged commit 51afa63 into apache:master May 18, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Jenkins This pull request looks good
Details

@lukaszlenart lukaszlenart deleted the lukaszlenart:locale-aware-converters branch May 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment