LANG-1172: Support dash as a delimiter in locales #766
Conversation
|
@c-w |
|
@garydgregory Done. |
|
@garydgregory Just following up on this. Is there anything further I can do to speed up getting this reviewed? Thanks in advance for your time! |
|
I will look this weekend. |
|
@garydgregory Do you have feedback on this pull request? Anything holding this back from getting merged? |
I need to take the time to review, hopefully sometime this week. |
There was a problem hiding this comment.
I am not sure whether we should modify the current methods to support this locale, or have separate methods. But the code looks really good 👍 Maybe someone else can chime in and share their thoughts on supporting both here. Thanks for the PR @c-w !
|
@kinow Given that the change is backwards compatible I'd say introducing a new method would be more confusing as the developer would have to know/remember to pick the correct function. @garydgregory Any thoughts or other comments on the PR? |
| @@ -248,7 +250,9 @@ private static Locale parseLocale(final String str) { | |||
| return new Locale(str); | |||
| } | |||
|
|
|||
| final String[] segments = str.split("_", -1); | |||
| final String[] segments = str.indexOf(DASH) != -1 | |||
| ? str.split(String.valueOf(DASH), -1) | |||
There was a problem hiding this comment.
Does it matter that dash is tested before underscore?
There was a problem hiding this comment.
It's been a while since I wrote this code, but I don't think so. Should be possible to switch them around without issues.
There was a problem hiding this comment.
All the unit tests still pass with the comparison swapped. I made the change in 0319d84. It's probably the right thing to optimize for the previous delimiter as that'll be the vast majority of use-cases.
|
Another friendly ping @garydgregory @kinow @sgoeschl. Could you take another look and let me know if you have any concerns about this change? Thanks in advance! |
|
Thanks for the merge! 🎉 |
|
You should build git master locally to make sure you can use the jar in your app. |
|
This change has resulted in some broken expectations in the differences between locale and language tags. While there is probably value in having a method that is open to taking either in order to produce a Locale object, it is not clear that should be this method. In fact, the original and current comments for the method say "This method takes the string format of a locale and creates the locale object from it". In addition, the rationale for why this specific change was made appears to based on some inaccurate understanding - BCP 47 is the spec for language tags, not locales and BCP47 does not support 'underscore', which was this method only supported before this change. |
|
Any update? |
|
Hi @seadbrane |
|
The PR is to revert this commit unless the intention has changed since the method was created.
On Feb 6, 2024, at 15:34, Gary Gregory ***@***.***> wrote:
Hi @seadbrane<https://github.com/seadbrane>
Feel free to provide a PR.
—
Reply to this email directly, view it on GitHub<#766 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AYLRVNIOZ3JZ7MZRIUE2RILYSK4V7AVCNFSM45YYDHH2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOJTGA4TINRUG42A>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
pr to mostly revert this change. |
This pull request is a partial fix for LANG-1172 adding support for dashes as delimiters for locales as per BCP47 thus enabling parsing of locales such as
en-GBin addition to the previously supported (but not to spec)en_GB.