Quantcast

[GitHub] struts pull request #138: WW-3171 WW-3357 WW-3650 WW-4581: Locale aware conv...

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts pull request #138: WW-3171 WW-3357 WW-3650 WW-4581: Locale aware conv...

lukaszlenart-2
GitHub user lukaszlenart opened a pull request:

    https://github.com/apache/struts/pull/138

    WW-3171 WW-3357 WW-3650 WW-4581: Locale aware converters

    This PR introduces Locale aware conversion of `BigDecimal` and `Double` and `Double`
   
    [WW-3171](https://issues.apache.org/jira/browse/WW-3171)
    [WW-3357](https://issues.apache.org/jira/browse/WW-3357)
    [WW-3650](https://issues.apache.org/jira/browse/WW-3650)
    [WW-4581](https://issues.apache.org/jira/browse/WW-4581)

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/lukaszlenart/struts locale-aware-converters

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/struts/pull/138.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #138
   
----
commit f874f9cde56f74c5161b17e645f779805c51a04b
Author: Lukasz Lenart <[hidden email]>
Date:   2017-05-12T10:19:48Z

    WW-4581 Uses proper logic to convert String to BigDecimal

commit 266d78d32c786276f37ae701267f6719ea9f8a75
Author: Lukasz Lenart <[hidden email]>
Date:   2017-05-12T11:48:14Z

    WW-3650 Supports double conversion for different locale

commit 20eced95008a9e08a20a59f287115ede00268897
Author: Lukasz Lenart <[hidden email]>
Date:   2017-05-12T12:40:20Z

    WW-3171 Converts numbers to strings using locale

commit 229afea64e77c2dba9eec62b2c339e9fc92c9ec7
Author: Lukasz Lenart <[hidden email]>
Date:   2017-05-12T13:58:54Z

    WW-3171 Uses proper number of digits when formatting double

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #138: WW-3171 WW-3650 WW-4581: Locale aware converters

lukaszlenart-2
Github user aleksandr-m commented on the issue:

    https://github.com/apache/struts/pull/138
 
    Will this break apps with locales which are using different decimal separator? E.g. in db `123.456` with locale which uses `,` as a decimal separator.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #138: WW-3171 WW-3650 WW-4581: Locale aware converters

lukaszlenart-2
In reply to this post by lukaszlenart-2
Github user lukaszlenart commented on the issue:

    https://github.com/apache/struts/pull/138
 
    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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #138: WW-3171 WW-3650 WW-4581: Locale aware converters

lukaszlenart-2
In reply to this post by lukaszlenart-2
Github user cnenning commented on the issue:

    https://github.com/apache/struts/pull/138
 
    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 😉


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #138: WW-3171 WW-3650 WW-4581: Locale aware converters

lukaszlenart-2
In reply to this post by lukaszlenart-2
Github user lukaszlenart commented on the issue:

    https://github.com/apache/struts/pull/138
 
    `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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #138: WW-3171 WW-3650 WW-4581: Locale aware converters

lukaszlenart-2
In reply to this post by lukaszlenart-2
Github user cnenning commented on the issue:

    https://github.com/apache/struts/pull/138
 
    > 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?



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #138: WW-3171 WW-3650 WW-4581: Locale aware converters

lukaszlenart-2
In reply to this post by lukaszlenart-2
Github user lukaszlenart commented on the issue:

    https://github.com/apache/struts/pull/138
 
    Sure thing, please register an issue


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #138: WW-3171 WW-3650 WW-4581: Locale aware converters

lukaszlenart-2
In reply to this post by lukaszlenart-2
Github user aleksandr-m commented on the issue:

    https://github.com/apache/struts/pull/138
 
    What about `Float` and `float`? I remember writing custom converter for it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #138: WW-3171 WW-3650 WW-4581: Locale aware converters

lukaszlenart-2
In reply to this post by lukaszlenart-2
Github user lukaszlenart commented on the issue:

    https://github.com/apache/struts/pull/138
 
    @aleksandr-m good point, done


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #138: WW-3171 WW-3650 WW-4581: Locale aware converters

lukaszlenart-2
In reply to this post by lukaszlenart-2
Github user lukaszlenart commented on the issue:

    https://github.com/apache/struts/pull/138
 
    Any other objections? I would start with what we have here and improve


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts issue #138: WW-3171 WW-3650 WW-4581: Locale aware converters

lukaszlenart-2
In reply to this post by lukaszlenart-2
Github user cnenning commented on the issue:

    https://github.com/apache/struts/pull/138
 
    👍 for merging



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] struts pull request #138: WW-3171 WW-3650 WW-4581: Locale aware converters

lukaszlenart-2
In reply to this post by lukaszlenart-2
Github user asfgit closed the pull request at:

    https://github.com/apache/struts/pull/138


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Loading...