-
Notifications
You must be signed in to change notification settings - Fork 325
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
i18 support for spanish and english locales are added #1021
base: master
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,70 @@ | |||
$ref = {0}: tiene un error con ''refs'' |
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 encoding
test in LocaleTest
needs to be modified to ensure that the encoding of this file works for Java 8.
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.
Could you please le me know how to do that?
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.
You need to modify this test.
json-schema-validator/src/test/java/com/networknt/schema/LocaleTest.java
Lines 118 to 125 in ffec188
void encoding() { | |
Map<String, String> expected = new HashMap<>(); | |
expected.put("ar","$: يجب أن يكون طوله 5 حرفًا على الأكثر"); | |
expected.put("cs","$: musí mít maximálně 5 znaků"); | |
expected.put("da","$: må højst være på 5 tegn"); | |
expected.put("de","$: darf höchstens 5 Zeichen lang sein"); | |
expected.put("fa","$: باید حداکثر 5 کاراکتر باشد"); | |
expected.put("fi","$: saa olla enintään 5 merkkiä pitkä"); |
You also need to modify this to add the supported locale
json-schema-validator/src/main/java/com/networknt/schema/i18n/Locales.java
Lines 33 to 35 in ffec188
public static final String[] SUPPORTED_LANGUAGE_TAGS = new String[] { "ar", "cs", "da", "de", "fa", "fi", "fr", | |
"iw", "he", "hr", "hu", "it", "ja", "ko", "nb", "nl", "pl", "pt", "ro", "ru", "sk", "sv", "th", "tr", "uk", | |
"vi", "zh-CN", "zh-TW" }; |
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.
Okay i will do that and let you know thank you @justin-tay
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1021 +/- ##
============================================
+ Coverage 78.90% 79.15% +0.24%
- Complexity 1965 1986 +21
============================================
Files 172 174 +2
Lines 6352 6427 +75
Branches 1255 1261 +6
============================================
+ Hits 5012 5087 +75
+ Misses 867 864 -3
- Partials 473 476 +3 ☔ View full report in Codecov by Sentry. |
|
||
// In later JDK versions the numbers will be formatted | ||
Map<String, String> expectedAlternate = new HashMap<>(); | ||
expectedAlternate.put("ar","$: يجب أن يكون طوله ٥ حرفًا على الأكثر"); | ||
expectedAlternate.put("fa","$: باید حداکثر ۵ کاراکتر باشد"); | ||
expectedAlternate.put("es","$: debe tener como máximo 5 caracteres"); |
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.
What is this comparison for and how is this possible? The rest of the alternate comparisons is because later JDK versions perform mapping to arabic numerals.
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.
Note that for Java 8 the properties file must be encoded in ISO 8859-1. UTF-8 is only supported from Java 9 onwards. If the test fails for Java 8 but passes for the rest it means that your file is not encoded in ISO 8859-1.
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.
@justin-tay
in the English text "Hello, how are you?" is encoded to ISO 8859-1 and then decoded back to a string. However, since ISO 8859-1 does not include characters specific to Spanish, the output will be the same as the input.
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.
If the encoding is correct then remove the expectedAlternate line and the test should pass. If not it's just not encoded correctly and is being corrupted to this máximo
text. This means that anyone using Java 8 will see debe tener como máximo 5 caracteres
as the translated text which is obviously not correct.
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.
@justin-tay do you prefer any tool for conversion. so that i can verify once more.
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 don't prefer any tool, it just need to be encoded such that Java 8 can load it properly. I just use Eclipse which will automatically convert text into ISO 8859-1 when pasted into a properties file as that is the default configuration in Eclipse for the Java Properties File content type.
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.
@justin-tay i tried the same i set my intelij editor to ISO 8859-1, i pasted the converted file and committed now still java 8 is failing. can you please do me a favour for this
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.
@justin-tay can you please give me the converted file. its needed for us
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.
@justin-tay please let me any help.
these two are needed for us and we are using it in our production please merge asap and provide the release