-
Notifications
You must be signed in to change notification settings - Fork 758
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
added i18n #549
base: uinverse
Are you sure you want to change the base?
added i18n #549
Conversation
…iseEdition into uinverse � Conflicts: � src/main/java/com/seriouscompany/business/java/fizzbuzz/packagenamingpackage/impl/Constants.java
good job! |
Nice work 👌 |
Well done! |
Wouldn't it be better to add resource bundles so it could be easily extended? |
Of course, maybe I will use ResourceBundle class to remake it later.
Get Outlook for iOS<https://aka.ms/o0ukef>
…________________________________
From: MikhailNikitinTR ***@***.***>
Sent: Saturday, April 2, 2022 3:04:29 AM
To: EnterpriseQualityCoding/FizzBuzzEnterpriseEdition ***@***.***>
Cc: chebyshevvv ***@***.***>; Comment ***@***.***>
Subject: Re: [EnterpriseQualityCoding/FizzBuzzEnterpriseEdition] added i18n (PR #549)
Wouldn't it be better to add resource bundles so it could be easily extended?
—
Reply to this email directly, view it on GitHub<#549 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AW7SBQXTTRFPCVC6XMFSMDLVC5CD3ANCNFSM5KFFOAVQ>.
You are receiving this because you commented.Message ID: ***@***.***>
|
@@ -19,5 +19,8 @@ public class Constants { | |||
public static final int INTEGER_ORIGIN_ZERO_VALUE = 0; | |||
public static final int LOOP_INC_VALUE = 1; | |||
public static final int LOOP_INIT_VALUE = 1; | |||
public static final String I18N_LIB_PROPERTIES_PATH = "assets/configuration/i18n/i18n_lib.properties"; | |||
public static final String LANGUAGE = "French"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the language hardcoded to French
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just there a default language was necessary then I gave a random.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BLOCKER: This is not enterprise code quality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How to set the default language ? What about let default language follow the system language that FizzBuzzEnterpriseEdition detected ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then , how to change language while the FizzBuzzEnterpriseEdition don't need to rerun?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not an expert on the l18n topic but someone could implement a FizzBuzzLanguageDetectorFactory which will return a FizzBuzzSystemLanguageDetector which then returns the String for the Language.
You would call that in the BuzzStringPrinter and pass the detected Language into the FizzBuzzOutputStrategyl18Proxy. Its really easy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds great.The FizzBuzzSystemLanguageDetector will detect the language after running .However ,what if the system language changes ?I'm afraid that the FizzBuzzSystemLanguageDetector detects at all times ,so it would be best that the FizzBuzzSystemLanguageDetector will be notified when the system language changes .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh thats a real big problem here. If i understand you properly you are unsure how to handle the language change if the FizzBuzz runs over a long time? This is a serious issue. The language should change indeed, so that the calcucation progress of the fizzbuzz software is not lost. The easist way we could archive this is that we exchange the constant language with a proper variable which is not constant. Good point.
Fizz.Chinese=嘶嘶 | ||
Fizz.English=Fizz | ||
Fizz.French=Cloques | ||
Fizz.German=FizzGenericName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no translation for eigennamen in german. The FizzBuzz application is called fizz buzz in german so you could simple use Fizz here.
Lets do some focus time on this big issue. Marketing will be impressed and the business can be scaled internationally if we succeed here. If that happens the company will have enough money for the long needed new coffee machine. |
**/ | ||
@Override | ||
public void output(String output) throws IOException { | ||
Properties properties = PropertiesLoaderUtils.loadAllProperties(Constants.I18N_LIB_PROPERTIES_PATH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the Spring documentation, this method always uses ISO-8859-1, for compatibility with the 1990s. Did you verify that this code works properly on Microsoft Windows, printing the intended messages instead of mojibake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good idea. Many of our users are in 1995, thought the majority of our users are in 2007.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Russian translation is missing. It's important, since Russia is the biggest country in terms of territory. Please add it:
Fizz.Russian=Физз
Buzz.Russian=Базз
added i18n