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

Create i18n enums for better localized string validation. #895

Merged
merged 1 commit into from Apr 18, 2018

Conversation

@smartboyathome
Copy link
Contributor

@smartboyathome smartboyathome commented Mar 31, 2018

This design tries to make our string localization more stable by forcing us to use enum constants. By doing this, we are able to write a test against it to verify that all strings exist, guaranteeing we won't get an exception when fetching the strings from the config file. Depending on how this design is taken, we may decide to incorporate some of the work from #892 and supersede that PR, or we may decide to merge that and rebase this.

@smartboyathome smartboyathome requested a review from Pr0methean Mar 31, 2018

public interface LocalizedString {
ResourceBundle STRINGS = ResourceBundle.getBundle("strings");

This comment has been minimized.

@Pr0methean

Pr0methean Mar 31, 2018
Contributor

Add String format(Object... args) and a void log(Object... args) that knows the appropriate log level.

@Pr0methean
Copy link
Contributor

@Pr0methean Pr0methean commented Mar 31, 2018

Should the enum values maybe be sorted by subject matter rather than by log level?

@Pr0methean Pr0methean requested a review from mastercoms Mar 31, 2018
@Pr0methean
Copy link
Contributor

@Pr0methean Pr0methean commented Mar 31, 2018

Also, how hard would it be to do the conversion if #892 was merged first? You'd just have to convert static methods to instance methods, but you'd have to be able to look up the enum entries by their key at edit-time (which I don't think IntelliJ IDEA can do, unless there's a plugin specifically for this design pattern; but I don't know what tools you're using).

@aramperes
Copy link
Member

@aramperes aramperes commented Mar 31, 2018

Although I think this is fine, how about making the interfaces be subinterfaces of LocalizedString? e.g. LocalizedString.Console.Info.IMPORT

@smartboyathome
Copy link
Contributor Author

@smartboyathome smartboyathome commented Apr 1, 2018

Pr0methean, It won't be the end of the world if #892 gets merged before this does, it'll just force me to redo all the conversions over to using these implementations since every single one will be in conflict. Upside is, as you said, it would get me the log* and format methods.

Momo, I figured out a way to do that, but the test I wrote will need to be updated to use reflection in order to gather all the LocalizedString instances. I'm working on this change right now.

This allows us to avoid exceptions due to misspelled keys through a set of tests.
@smartboyathome smartboyathome force-pushed the smartboyathome:i18n-enums branch from f4e84aa to 902a1a3 Apr 8, 2018
@aramperes aramperes merged commit 0b5b85e into GlowstoneMC:dev Apr 18, 2018
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants