Use "de" in Romanian for numbers >= 20 #235
Conversation
|
I'll review this in the next two weeks or so. Thank you.
|
EvanHahn
left a comment
EvanHahn
left a comment
There was a problem hiding this comment.
This mostly looks good, but I have a few small requests. Thank you!
humanize-duration.js
Outdated
| ["minut", "minute"], | ||
| ["secundă", "secunde"], | ||
| ["milisecundă", "milisecunde"], | ||
| ro: romanianLanguage( |
There was a problem hiding this comment.
Nitpick: I'd prefer if this could use language() instead, and remove the romanianLanguage() function. It should just be a matter of inlining the contents of romanianLanguage().
test/humanizer.js
Outdated
| assert.strictEqual(h(123), "Zero.OneTwoThree seconds"); | ||
| }); | ||
|
|
||
| it("handles Romanian plural forms with 'de' correctly", function () { |
There was a problem hiding this comment.
Is there a way to test this inside of test/definitions/ro.tsv instead?
There was a problem hiding this comment.
I put here whatever I couldn't test using the tsv because numbers are too big and would roll into the next unit, e.g. 100 days would be 3 months etc. I can remove these tests if you want, I'm OK with the coverage from ro.tsv.
There was a problem hiding this comment.
I think I'd prefer if these tests were removed. Happy to discuss further if you disagree.
humanize-duration.js
Outdated
| if (c === 1) { | ||
| return unit[0]; | ||
| } | ||
| // Non-integers and 0 use plural without "de" |
There was a problem hiding this comment.
Nitpick: could you remove this comment?
| // Non-integers and 0 use plural without "de" |
|
I'll take another look soon. |
EvanHahn
left a comment
EvanHahn
left a comment
There was a problem hiding this comment.
Looks good other than one small comment! Thank you!
test/humanizer.js
Outdated
| assert.strictEqual(h(123), "Zero.OneTwoThree seconds"); | ||
| }); | ||
|
|
||
| it("handles Romanian plural forms with 'de' correctly", function () { |
There was a problem hiding this comment.
I think I'd prefer if these tests were removed. Happy to discuss further if you disagree.
|
Thanks! I'll deploy this soon. |
|
This has been released in |
|
Hey @EvanHahn, is there a process for updating the code used in the demo page? It looks like the version in the |
|
I always forget to update that. I'll fix in the next few days. |
|
Updated! |
|
Thanks! |
2 participants
The current Romanian translation is slightly wrong. Similar to Slavic languages, Romanian has different rules for how plurals look:
1 <= n % 100 <= 19"de"+ noun otherwise(See Wikipedia.)