Skip to content
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

Partially revert LANG-1172. #1271

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

seadbrane
Copy link

The changes for LANG-1172 changed the allowed input and expectations of LocaleUtils.toLocale to now also support strings which include '-'. This change reverts that behavior, but leaves support for it through a protected method.

@garydgregory
Copy link
Member

garydgregory commented Sep 3, 2024

@seadbrane
Copy link
Author

seadbrane commented Sep 3, 2024

@garydgregory Here is the earlier comment on made on the original pr.

#766 (comment)

Making this method to also support '-' changes the behavior, and now breaks code that were using it to disambiguate between locale strings and language tag strings. While I do think it would be useful to have a method to take either, that should be a new method in order to keep the behavior of this method consistent with the original intent

@seadbrane
Copy link
Author

Any update? Also, I did not expect this to be the final revision - as I wasn't sure the preferred method signature for supporting strings with '-', which is why I left this as a protected method now.

Options:

  1. Make the toLocale(String, boolean) method public
  2. Expose a new method such as toLocaleLenient(String)

@seadbrane
Copy link
Author

I can understand the reluctance to rolling this back, as I am sure there are now folks relying on the new behavior.
Although in addition to breaking the previous contract, the current behavior muddies the waters on the differences between locale and language tags, and I am concerned that more folks will now think of them and try to treat them as equivalent - which is not the case conceptually or from a string representation perspective.

@garydgregory
Copy link
Member

garydgregory commented Sep 10, 2024

Hi @seadbrane
If you want this issue to get more visibility, you should consider posting to the developer's mailing list https://commons.apache.org/mail-lists.html

@seadbrane
Copy link
Author

seadbrane commented Sep 16, 2024

Hi @garydgregory
I see you posted something on the email list already - thank you. I was not subscribed to the list at the time and looks like no responses- mind pinging the thread, or is it preferred to start a new one?

Related: The javadoc for this method had the following up through 3.12.0.

"This method validates the input strictly. The language code must be lowercase. The country code must be uppercase. The separator must be an underscore. The length must be correct. "

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.

2 participants