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

refactor: move declaration for english 'ordinal' to 'en' locale file #2071

Merged
merged 2 commits into from
Oct 9, 2022

Conversation

BePo65
Copy link
Contributor

@BePo65 BePo65 commented Sep 26, 2022

All locale files have a definition for 'ordinal' except the 'en' one. For this locale the definition of 'ordinal' is part of the 'AdvancedFormat' plugin. This is inconsistent not quite logical.

This pr solves this topic and should solve issue #1891

@codecov
Copy link

codecov bot commented Sep 26, 2022

Codecov Report

Merging #2071 (74144ff) into dev (c9370ea) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               dev     #2071   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          181       182    +1     
  Lines         2080      2080           
  Branches       547       547           
=========================================
  Hits          2080      2080           
Impacted Files Coverage Δ
src/locale/en.js 100.00% <100.00%> (ø)
src/plugin/advancedFormat/index.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@iamkun
Copy link
Owner

iamkun commented Oct 9, 2022

Thanks.

But there's another problem, format('Do') function is also added in advancedFormat plugin, that is to say, even if added ordinal to 'en', we still can't get correct format('Do') result without advancedFormat plugin 🤔

@BePo65
Copy link
Contributor Author

BePo65 commented Oct 9, 2022

Of course that is correct. But IMO code / architecture should be consistent.
By now we have the situation that for 'en' we have the definition of the ordinal function in the plugin AdvancedFormat and for all other locales we have it in the corresponding locale file.

If someone want to do format('Do'), he needs AdvancedFormat, so this does not make a functional difference, but architecture is more than pure functionality (I am sure you agree, if you look at my programs from 30 years ago 😄).

@iamkun
Copy link
Owner

iamkun commented Oct 9, 2022

well, that sounds reasonable, let's get it merged.

@iamkun iamkun merged commit 7ae1a1d into iamkun:dev Oct 9, 2022
@BePo65 BePo65 deleted the fix/issue1891 branch October 9, 2022 15:36
@iamkun
Copy link
Owner

iamkun commented Oct 21, 2022

🎉 This PR is included in version 1.11.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants