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

[locale] Extend cs locale with name of the months in genitive #4771

Merged
merged 1 commit into from Jan 21, 2019

Conversation

saxicek
Copy link
Contributor

@saxicek saxicek commented Sep 10, 2018

In Czech, it is possible to use also declined form of the month name instead of the nominative case.
This commit adds those declined names of the months.

@radekdostal @marxsk @petrbela - please verify this patch as locales have to be verified by native speakers.

This PR is meant to replace solution suggested in #4765 (and agreed with @marxsk in #4765 (comment))

Discussion:
Pro of this solution is that is it easier to understand because plain regexes or their arrays are used instead of anonymous function.
Con is that "červenec" must precede "červen" in the monthsRegex and monthsStrictRegex to match properly.

@jsf-clabot
Copy link

jsf-clabot commented Sep 10, 2018

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Sep 10, 2018

Coverage Status

Coverage increased (+0.003%) to 88.138% when pulling c225a7a on saxicek:patch-1 into c68b4f1 on moment:develop.

@saxicek
Copy link
Contributor Author

saxicek commented Sep 10, 2018

Travis failed on locale/ka.js - which was untouched by this PR.

@petrbela
Copy link
Contributor

I haven't kept up with how monthsStrict works these days. Is the inflected version automatically used after an ord date (like "1. cervna")?

I feel like we should add a test case for the format function (is it the LL format?), or whichever publicly available interface this change affects.

Con is that "červenec" must precede "červen" in the monthsRegex and monthsStrictRegex to match properly.

I'm okay with this as long as we add a comment to the source file why this workaround is necessary.

@saxicek
Copy link
Contributor Author

saxicek commented Sep 22, 2018

@petrbela This change affects date parsing, not formatting. Therefore not sure about your suggestion to add test case for the format function.

Addressed the request to document why months are not in order in the regular expressions.

Unfortunately tests are still failing due to #4762.

@petrbela
Copy link
Contributor

Ah right makes sense. I'm just wondering whether the functionality should be symmetric; if we can parse a specific format, we should be able to print (format) it, too. As far as I can see, that's not available right now. What do you think?

@saxicek
Copy link
Contributor Author

saxicek commented Sep 22, 2018

Yes, it does make sense. But unfortunately I am unable to dedicate more time to design and implement this. Any volunteers?

@marxsk
Copy link

marxsk commented Sep 22, 2018

imho the impact of having function symmetric is not that big. Additionally, it makes sense to make this available also for the other languages. And it is possible that in various languages this 'genitive-case' is oversimplifaction.

@saxicek
Copy link
Contributor Author

saxicek commented Sep 24, 2018

The thing I don't know is how would you switch between the 2 forms - new setting? Where? On the other hand if someone needs that then (s)he can just simply overload locale definition using:

moment.updateLocale('cs', {
    months : 'ledna_února_března_dubna_května_června_července_srpna_září_října_listopadu_prosince'.split('_')
});

You could also introduce new locale variant but still - I don't think it is worth the effort.

@marwahaha
Copy link
Member

marwahaha commented Dec 13, 2018

@saxicek what's the status of this? looks like you have approval. Also, are there changes that break existing users of this locale?

@saxicek
Copy link
Contributor Author

saxicek commented Dec 13, 2018

Yes. I think those involved in the discussion are ok with the solution. We were waiting for #4762 to pass CI tests.

@marwahaha marwahaha merged commit 24e55df into moment:develop Jan 21, 2019
@saxicek saxicek deleted the patch-1 branch February 18, 2019 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants