-
-
Notifications
You must be signed in to change notification settings - Fork 51
Add Norwegian locale and corresponding test (failing as pr issue #146) #147
Add Norwegian locale and corresponding test (failing as pr issue #146) #147
Conversation
This PR has expanded according to the findings described in #146 |
First of all, thanks for the contribution @Dambakk ! I think we should have in mind several things: This is changing the current behaviour. This is something we can consider for 2.0 as a breaking change, or opt-in/out with a setting or something. We should ask people from different countries to validate the dates format and maybe also check against other libraries. Going to ask on slack/discord, also if people subscribed to this repo can check, let's see if people from other countries have feedback on this for their respective languages, so lets wait a few days for this to happen if you don't mind 👍 |
@@ -119,12 +137,12 @@ class KlockLocaleTest { | |||
fun testRussianLocale() { | |||
assertEquals( | |||
""" | |||
Ср, 13 Мар 2019 21:36:45 UTC | |||
13 Мар 2019 г. 21:36:45 | |||
ср, 13 мар 2019 21:36:45 UTC |
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 day of the week ("ср") should start with an upper letter ("Ср")
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.
Hi! Can you confirm that all weekdays in two-letter short form should start with an upper case letter?
override val daysOfWeekShort = listOf(
"вс", "пн", "вт", "ср", "чт", "пт", "сб"
)
Can you type them here so it gets 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.
Like that?
override val daysOfWeekShort = listOf(
"Вс", "Пн", "Вт", "Ср", "Чт", "Пт", "Сб"
)
Of course this can wait, @soywiz! I totally agree that this should be targeted for a major version. At least we are aware of the situation. I'm not a language person myself, but I hope the community will contribute with their knowledge. I can take responsibility to update this PR as we receive feedback. Can you point me to any documentation that needs to be updated? |
German is looking good. |
Dutch is looking good, officially the months/weekdays should not be capiticalized: https://onzetaal.nl/taaladvies/oktober-october/ |
@@ -173,12 +191,12 @@ class KlockLocaleTest { | |||
fun testUkrainianLocale() { | |||
assertEquals( | |||
""" | |||
Ср, 13 Бер 2019 21:36:45 UTC | |||
13 Бер 2019 р. 21:36:45 | |||
ср, 13 бер 2019 21:36:45 UTC |
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.
For Ukrainian, the day of the week should start with an upper letter: "Ср" instead of "ср".
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.
Hi!
Can you confirm that this is the correct two-letter short form for weekdays in Ukrainian?
override val daysOfWeekShort = listOf(
"нд", "пн", "вт", "Ср", "чт", "пт", "сб"
)
Середа, 13 Березня 2019 р. | ||
13 Березня 2019 р. | ||
13 Бер 2019 р. | ||
середа, 13 березня 2019 р. |
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 full format of the day of the week should start with an upper letter as well.
"Середа" instead of "середа"
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.
Hi!
Can you confirm that these weekdays are correct?
override val daysOfWeek = listOf(
"неділя", "понеділок", "вівторок", "Середа", "четвер", "п'ятниця", "субота"
)
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'm cross-checking with how Java DateTime does formatting and gets an exception:
Expected :середа, 18 січня 1995 21:36:45 // <- Java
Actual :Середа, 18 січня 1995 21:36:45 // <- Klock
Can you confirm that Java is wrong and that it should be uppercased?
Left few comments for Ukrainian locale. Weekdays should be capitalized. |
Thank you all for the help!
Klock documentation is on this file: Also I have been thinking. Would be nice to be consisent with what Java and other libraries do in the case the behaviour is the same or very similar. What would you think on adding a test checking different locales on the
I have also checked what does moment.js (not checked JVM), and seems to be different per language. Check the results here: https://jsfiddle.net/soywiz/j35yht7a/6/ for (let lang of ['nb_NO', 'en', 'de', 'es', 'fr', 'zh-cn', 'ja-jp', 'pt_PT', 'nl-NL', 'ru']) {
document.write('<p>' + lang + ": " + moment().locale(lang).format('dddd, MMMM Do YYYY, h:mm:ss a') + '</p>')
}
We could also check this: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat That is the standard i18n for Ecmascript |
That sounds like a great idea! I will look into it 👍 |
Strange, Portuguese is running fine on my computer. Both produce lowercased output. |
@Dambakk Nice. It's a pity that it didn't work consistently. If the behaviour is changed, maybe we should stick to specific literals, based on what's more common comparing with other libraries/frameworks like moment.js, the standardized version of ecmascript, several versions of the JVM, .NET, Swift. If you need help with that I can make samples in different languages and put here the code + output when I have some time. |
I was running java 12 so that might be it. IDEA crashed when I changed it to jdk 8 :P but since both the tests here and moment.js says something different than my computer I guess they are right ;) However, Russian is failing now.. moment.js and my computer say weekdays should start with lowercase letter, whilst java 8(?) say weekdays should start with uppercased.. What do you think? Anything else that you think should be done here? I can update the documentation once the PR is approved. |
Uhm. Tests are still failing. Maybe we should use string literals instead |
.NET: using System;
using System.Globalization;
namespace ConsoleApp1
{
class Program
{
static void Main(string[] args)
{
var date = new DateTime(2020, 10, 25, 14, 10, 13);
var format = "dddd, dd MMMM yyyy hh:mm:ss tt";
foreach (var lang in new[] {"nb_NO", "en", "de", "es", "fr", "zh-cn", "ja-jp", "pt_PT", "nl-NL", "ru"})
{
var str = date.ToString(format, CultureInfo.GetCultureInfo(lang));
Console.WriteLine($"{lang}: {str}");
}
}
}
}
|
In .NET looks similar to moment.js (english and german the only ones capitalized), except for Portuguese that is non-capitalized on .NET |
On standard modern JavaScript using Intl: let date = new Date(Date.UTC(2020, 9, 25, 14, 10, 13));
let options = {weekday: 'long', year: 'numeric', month: 'long', day: 'numeric', timeZone: 'UTC', timeZoneName: 'short'};
for (let lang of ['nb-NO', 'en', 'de', 'es', 'fr', 'zh-cn', 'ja-jp', 'pt-PT', 'nl-NL', 'ru']) {
try {
let str = new Intl.DateTimeFormat(lang, options).format(date);
console.log(`${lang}: ${str}`);
} catch (e) {
console.error(`Invalid locale ${lang}`);
}
}
Seems to work the same as on .NET, only English and German are capitalized |
Swift: import Foundation
let date = Date.init()
let dateFormatter = DateFormatter()
dateFormatter.dateStyle = .long
dateFormatter.timeStyle = .long
for lang in ["nb-NO", "en", "de", "es", "fr", "zh-cn", "ja-jp", "pt-PT", "nl-NL", "ru"] {
dateFormatter.locale = Locale.init(identifier: lang)
print(dateFormatter.string(from: date))
}
Again, in terms of capitalization, only english and german. So I would go that way |
Finally, the tests run successfully! Please go ahead with code review, @soywiz . |
klock/src/jvmTest/kotlin/com/soywiz/klock/KlockLocaleTestJvm.kt
Outdated
Show resolved
Hide resolved
LGTM Thanks! |
Fixes #146 |
See issue #146