Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove globally affecting call to Locale#setDefault #5141

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

BlvckBytes
Copy link
Contributor

Greetings!

The call to Locale#setDefault within LocaleLoader#initialize threw me for quite the massive loop today, as it affected multiple other plugins which assumed Locale#getDefault to be the JVM's, and thus the system's locale, which mcMMO hard-coded to en_US.

While I have to admit that those other developers don't quite adhere to best practises by depending on this global state either, I still wholeheartedly believe that mcMMO should refrain from altering JVM-global-state nonetheless.

Also, since this initialization may occur after executing some of the static-blocks of other classes containing various formatters, many test-cases failed to execute due to formatting-discrepancies on my local machine, having had a non-english locale configured.

My proposal represents merely a quick patch, as to cut out altering global state, but the ideal solution would obviously be to use the locale as configured on the plugin; that would demand a few architectural changes, which I do not quite have the resources for, at this point in time.

I hope that this tiny but still rather significant issue can be settled soon.

Best Regards,
BlvckBytes

…ed static locale-parameter to all instances of a Formatter
@nossr50
Copy link
Member

nossr50 commented Jan 25, 2025

Thank you for the write up and thorough explanation, and thank you for the PR as well.

@nossr50 nossr50 merged commit b36848b into mcMMO-Dev:master Jan 25, 2025
1 check passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants