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

[bugfix] remove ordinal for Turkish locale as they use `cardin… #4361

Merged
merged 2 commits into from Mar 2, 2018

Conversation

alan-agius4
Copy link
Contributor

Turkish don't use ordinal days but rather cardinals

#4122 (comment)

@jsf-clabot
Copy link

jsf-clabot commented Dec 20, 2017

CLA assistant check
All committers have signed the CLA.

@alan-agius4 alan-agius4 changed the title fix(local): remove ordinal for Turkish locale as they use `cardin… bugfix: remove ordinal for Turkish locale as they use `cardin… Dec 20, 2017
@alan-agius4 alan-agius4 changed the title bugfix: remove ordinal for Turkish locale as they use `cardin… [bugfix]: remove ordinal for Turkish locale as they use `cardin… Dec 20, 2017
@alan-agius4 alan-agius4 changed the title [bugfix]: remove ordinal for Turkish locale as they use `cardin… [bugfix] remove ordinal for Turkish locale as they use `cardin… Dec 20, 2017
@marwahaha
Copy link
Member

@BYK - can you review this?

It's unclear if we should keep the Turkish ordinals in the codebase at all (this PR removes them entirely).

Copy link
Contributor

@BYK BYK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think removing all ordinals is not the right approach since it results in incorrect formattings such as the one I highlighted. The new test incorrectly says "45 days of the year" which is grammatically correct but means something different.

['h hh', '3 03'],
['H HH', '15 15'],
['m mm', '25 25'],
['s ss', '50 50'],
['a A', 'pm PM'],
['[yılın] DDDo [günü]', 'yılın 45\'inci günü'],
['[yılın] DDDo [günü]', 'yılın 45 günü'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect. The old test is correct and we should keep that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the only spec that should be kept?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the original issue earlier, one never uses "December 3rd", it is always "3 December" or rarely, "December 3". Ordinals are only used when you are saying something like "The 3rd week of the year", "The 5th day of the month" (this one actually uses a slightly different thing than ordinals but ordinals are also fine) or "the first Sunday of March".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some further changes, can you re-check please? I am not sure if I covered everything.

@marwahaha
Copy link
Member

@alan-agius4 - could you add a couple tests specific to the issue you're fixing?

@BYK - could you review again?

@alan-agius4
Copy link
Contributor Author

I actually amended the existing test to match the new behaviour for some dates

Copy link
Contributor

@BYK BYK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

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

4 participants