You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2021/05/29 23:27:30 UTC
[GitHub] [commons-lang] c-w opened a new pull request #766: LANG-1172: Support dash as a delimiter in locales
c-w opened a new pull request #766:
URL: https://github.com/apache/commons-lang/pull/766
This pull request is a partial fix for [LANG-1172](https://issues.apache.org/jira/browse/LANG-1172) adding support for dashes as delimiters for locales as per [BCP47](https://tools.ietf.org/html/bcp47#section-4) thus enabling parsing of locales such as `en-GB` in addition to the previously supported (but not to spec) `en_GB`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] c-w commented on pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
c-w commented on pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#issuecomment-944367626
@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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] c-w commented on a change in pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
c-w commented on a change in pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#discussion_r763190834
##########
File path: src/main/java/org/apache/commons/lang3/LocaleUtils.java
##########
@@ -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)
Review comment:
All the unit tests still pass with the comparison swapped. I made the change in 0319d84d1. It's probably the right thing to optimize for the previous delimiter as that'll be the vast majority of use-cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] garydgregory commented on pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#issuecomment-939072265
@c-w
May you please rebase on master?
TY!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] c-w commented on a change in pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
c-w commented on a change in pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#discussion_r745230053
##########
File path: src/main/java/org/apache/commons/lang3/LocaleUtils.java
##########
@@ -324,7 +327,7 @@ public static Locale toLocale(final String str) {
throw new IllegalArgumentException("Invalid locale format: " + str);
}
final char ch0 = str.charAt(0);
- if (ch0 == '_') {
+ if (ch0 == '_' || ch0 == '-') {
Review comment:
Added the constant in 11a6d1d52. I'd prefer not to add the helper method to avoid changing too much of the structure as part of this fairly minor overall change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] garydgregory commented on pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#issuecomment-963413151
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] c-w commented on pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
c-w commented on pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#issuecomment-1059885900
Thanks for the merge! :tada:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] c-w commented on pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
c-w commented on pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#issuecomment-1012228057
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] garydgregory commented on pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#issuecomment-944411168
I will look this weekend.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] c-w commented on a change in pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
c-w commented on a change in pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#discussion_r763186990
##########
File path: src/main/java/org/apache/commons/lang3/LocaleUtils.java
##########
@@ -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)
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] c-w commented on a change in pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
c-w commented on a change in pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#discussion_r745229865
##########
File path: src/main/java/org/apache/commons/lang3/LocaleUtils.java
##########
@@ -248,7 +248,9 @@ private static Locale parseLocale(final String str) {
return new Locale(str);
}
- final String[] segments = str.split("_", -1);
+ final String[] segments = str.contains("-")
+ ? str.split("-", -1)
Review comment:
Done in 11a6d1d52.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] c-w commented on a change in pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
c-w commented on a change in pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#discussion_r763190834
##########
File path: src/main/java/org/apache/commons/lang3/LocaleUtils.java
##########
@@ -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)
Review comment:
All the unit tests still pass with the comparison swapped. I made the change in 0319d84d1. It's probably the right thing to do to optimize for the previous delimiter as that'll be the vast majority of use-cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] coveralls commented on pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#issuecomment-850914246
[](https://coveralls.io/builds/40146125)
Coverage increased (+0.0006%) to 94.977% when pulling **fb3497e44f9b5b800cdd714d390989b86a971900 on c-w:LANG-1172-LocaleUtilsDashDelimiter** into **bbc690f2cbcb9ebda89e18ed0f4745984888cac5 on apache:master**.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] c-w commented on pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
c-w commented on pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#issuecomment-939179214
@garydgregory Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] c-w commented on pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
c-w commented on pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#issuecomment-963280110
@garydgregory Do you have feedback on this pull request? Anything holding this back from getting merged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] sgoeschl commented on a change in pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
sgoeschl commented on a change in pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#discussion_r744933460
##########
File path: src/main/java/org/apache/commons/lang3/LocaleUtils.java
##########
@@ -248,7 +248,9 @@ private static Locale parseLocale(final String str) {
return new Locale(str);
}
- final String[] segments = str.split("_", -1);
+ final String[] segments = str.contains("-")
+ ? str.split("-", -1)
Review comment:
Blame my weak eyes - could we introduce constants here, e.g. "UNDERSCORE"?
##########
File path: src/main/java/org/apache/commons/lang3/LocaleUtils.java
##########
@@ -324,7 +327,7 @@ public static Locale toLocale(final String str) {
throw new IllegalArgumentException("Invalid locale format: " + str);
}
final char ch0 = str.charAt(0);
- if (ch0 == '_') {
+ if (ch0 == '_' || ch0 == '-') {
Review comment:
We could use "UNDERSCORE" or "DASH" here or introduce a helper method, e.g. "isDelimiter"? What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] garydgregory merged pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
garydgregory merged pull request #766:
URL: https://github.com/apache/commons-lang/pull/766
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] garydgregory commented on pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#issuecomment-1059896889
You should build git master locally to make sure you can use the jar in your app.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] c-w commented on pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
c-w commented on pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#issuecomment-986881732
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] garydgregory commented on a change in pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#discussion_r763142047
##########
File path: src/main/java/org/apache/commons/lang3/LocaleUtils.java
##########
@@ -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)
Review comment:
Does it matter that dash is tested before underscore?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] c-w commented on a change in pull request #766: LANG-1172: Support dash as a delimiter in locales
Posted by GitBox <gi...@apache.org>.
c-w commented on a change in pull request #766:
URL: https://github.com/apache/commons-lang/pull/766#discussion_r745230053
##########
File path: src/main/java/org/apache/commons/lang3/LocaleUtils.java
##########
@@ -324,7 +327,7 @@ public static Locale toLocale(final String str) {
throw new IllegalArgumentException("Invalid locale format: " + str);
}
final char ch0 = str.charAt(0);
- if (ch0 == '_') {
+ if (ch0 == '_' || ch0 == '-') {
Review comment:
Added the constant in 730e156. I'd prefer not to add the helper method to avoid changing too much of the structure as part of this fairly minor overall change.
##########
File path: src/main/java/org/apache/commons/lang3/LocaleUtils.java
##########
@@ -248,7 +248,9 @@ private static Locale parseLocale(final String str) {
return new Locale(str);
}
- final String[] segments = str.split("_", -1);
+ final String[] segments = str.contains("-")
+ ? str.split("-", -1)
Review comment:
Done in 730e156.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org