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

Use Locale.ROOT in String.format() #143

Merged
merged 1 commit into from
Sep 8, 2021
Merged

Use Locale.ROOT in String.format() #143

merged 1 commit into from
Sep 8, 2021

Conversation

jorsol
Copy link
Contributor

@jorsol jorsol commented Sep 6, 2021

Is generally a bad practice to depend on the default platform locale, so this PR use Locale.ROOT for String.format() calls.

Signed-off-by: Jorge Solórzano <jorsol@gmail.com>
@Ladicek
Copy link
Contributor

Ladicek commented Sep 7, 2021

I agree that depending on the platform locale is generally considered bad, but calling String.format("my format string", my, parameters) is super common and adding the locale argument just adds visual clutter. I'm not really sure we should do this.

@jorsol
Copy link
Contributor Author

jorsol commented Sep 7, 2021

Is the sad thing about Java defaults, but just because is super common doesn't mean it should be the correct option.

I agree that the locale argument adds visual clutter, but it potentially reduces the risk of the defaults:

So while in this case is not really critical as is just formating the output messages, it makes the output consistent.

Feel free to close it if the visual clutter is more annoying than the potential benefit.

@Ladicek
Copy link
Contributor

Ladicek commented Sep 8, 2021

I slept on it and came to the conclusion that if we really want to use String.format as poor man's string interpolation, then we should just do it properly. Thanks for the PR!

@Ladicek Ladicek added this to the 2.4.1.Final milestone Sep 8, 2021
@Ladicek Ladicek merged commit 16172a8 into smallrye:master Sep 8, 2021
@jorsol jorsol deleted the use-locale-root-string-format branch May 3, 2022 14:18
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