Preparse postformat + implementation in ar locale + tests #1255

VehpuS · 2020-12-06T01:02:00Z

Added a plugin to implement the preparse / postformat functions for locales such as "ar", based on the moment implementation. I added tests for both the plugin and the locale changes based on the moment tests.

This should solve #416

Do note that the relative time implementation for the ar locale does not match the moment one, so the tests for the locale are different (to focus on coverage and not correctness tests - since I am not an arabic speaker and could not fix the relative time implementation in the scope of this change).

Update recent changes in main branch for merging to dev branch

codecov · 2020-12-06T01:03:39Z

Codecov Report

Merging #1255 (e565267) into dev (9544ed2) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               dev     #1255   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          174       175    +1     
  Lines         1653      1697   +44     
  Branches       374       387   +13     
=========================================
+ Hits          1653      1697   +44

Impacted Files	Coverage Δ
src/constant.js	`100.00% <100.00%> (ø)`
src/index.js	`100.00% <100.00%> (ø)`
src/locale/ar.js	`100.00% <100.00%> (ø)`
src/plugin/customParseFormat/index.js	`100.00% <100.00%> (ø)`
src/plugin/duration/index.js	`100.00% <100.00%> (ø)`
src/plugin/preParsePostFormat/index.js	`100.00% <100.00%> (ø)`
src/plugin/relativeTime/index.js	`100.00% <100.00%> (ø)`
src/plugin/timezone/index.js	`100.00% <100.00%> (ø)`
src/locale/he.js	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9544ed2...e565267. Read the comment docs.

VehpuS · 2020-12-23T22:22:45Z

@iamkun just making sure: is there something that needs fixing / changing before this can be committed?

iamkun · 2020-12-24T06:01:59Z

First of all, thanks for this PR.

One thing I did not get clearly is, seems it changed the original code too much to support this feature.

And I'm afraid I did not know this issue much.

Any more detail you can give us, please? Or is it better for us to leave it as a separate plugin package?

VehpuS · 2020-12-24T06:33:02Z

Hi @iamkun - the changes to the original code are because I did a merge commit to my branch following a change you've made to the dev branch (commit acd8736). I did it to ensure my code will merge well with the branch. I can try to create a new pull request without the merge commit if it's confusing - I was only trying to ensure tests pass with the most recent code. I don't think it should mess anything up frankly...

The only manual change I made to existing (plugin) code was to the relative time plugin, where I made an inner logic overridable through the prototype chain to support postFormatting (see fromToBase in the relativeTime changes).

Let me know how you want me to proceed :)

iamkun · 2020-12-24T06:41:13Z

Ok, I see.

How should we use this plugin?

As far as I can see, if I want to get the correct ar locale in relative time, should we enable preParsePostFormat plugin first?

It's a little bit wired to me, to be honest.

VehpuS · 2020-12-24T07:27:51Z

Ok, I see.

How should we use this plugin?

As far as I can see, if I want to get the correct ar locale in relative time, should we enable preParsePostFormat plugin first?

It's a little bit wired to me, to be honest.

The plugin works fine with or without the relative time component. It is not a requirement.

iamkun · 2020-12-24T07:39:07Z

Then what's this plugin used for? I've checked the code and think it might be related to ar locale relative time format string.

Correct me if I am wrong.

VehpuS · 2020-12-24T07:57:15Z

Now I understand the confusion.

Preparse / postformat is a feature in moment that lets you process the input before the parser and process the string output after the formatter.

In the AR locale specifically, it is used to support Arabic numerals (see the last two commits). I needed to include support for the relative time plugin because in moment it is also processed by this logic.

VehpuS · 2020-12-24T07:58:14Z

It is also used in other locales an I personally used it in a custom locale for work on moment

iamkun · 2020-12-24T08:59:15Z

Thanks for this background information.

As I am not familiar with the concept, could you please give some reference of moment.js in this topic we've talked about?

This could give me a more detailed understanding of this PR.

VehpuS · 2020-12-24T10:17:48Z

In the docs: https://momentjs.com/docs/#/i18n/locale-data/

In the code: https://github.com/moment/moment/blob/b7ec8e2ec068e03de4f832f28362675bb9e02261/moment.js
(search for preparse / postformat - there aren't many uses in the code)

Moment.js test (on which I based the dayjs tests - commit
0211f07): https://github.com/moment/moment/blob/develop/src/test/moment/preparse_postformat.js

iamkun · 2020-12-25T03:23:22Z

THX. I'll take a look at the doc you've provied.

VehpuS · 2020-12-30T22:13:21Z

THX. I'll take a look at the doc you've provied.

Did you get a chance to look at the references?

iamkun · 2020-12-31T05:41:45Z

src/plugin/preParsePostFormat/index.js

+export default (option, dayjsClass, dayjsFactory) => {
+  // This plugin depends on other plugins - so I will import them here
+  // equivalent to dayjsClass.extend(duration)
+  localeData(option, dayjsClass, dayjsFactory)


we don't have to import other plugins like this.

Just listing the dependent plugin in the doc.

iamkun · 2020-12-31T05:43:04Z

Yes, it looks good to me. If I want to get the correct ar locale result, should I use it in this way

dayjs.extend(localeData)
dayjs.extend(preParsePostFormat)
dayjs.loclae('ar')

dayjs().formNow()

VehpuS · 2020-12-31T10:34:48Z

Yes, it looks good to me. If I want to get the correct ar locale result, should I use it in this way
dayjs.extend(localeData)
dayjs.extend(preParsePostFormat)
dayjs.loclae('ar')

dayjs().formNow()

You actually don't need to extend localeData, the plugin takes care of it for you.

The ar tests I've added demonstrate the usage of the plugin (a5b9200).

iamkun · 2021-01-01T06:11:57Z

Yes, you can see the comment above, I don't think it's a good idea to import localeData plugin into this plugin

VehpuS · 2021-01-01T12:54:27Z

Yes, you can see the comment above, I don't think it's a good idea to import localeData plugin into this plugin

I don't mind fixing this (it's quite simple) but I think it's a bad idea for two reasons:

The plugin cannot function without loading localeData - so it either becomes irrelevant without the other plugin or requires replicating logic from the localeData plugin to read the additional data from the locale file.
If we require the user to write something every time they use the plugin, isn't it preferable to save them the trouble?

Let me know what you think - if you insist I will of course conform to your decision :)

Best regards and happy new year!

iamkun · 2021-01-01T14:48:43Z

Cause by design, a plugin could be dependent on another one.

https://day.js.org/docs/en/plugin/advanced-format

Check format Week of year as an example here.

…explicitly

VehpuS · 2021-01-01T22:46:46Z

Cause by design, a plugin could be dependent on another one.

https://day.js.org/docs/en/plugin/advanced-format

Check format Week of year as an example here.

As I said - it's a pretty simple fix. I've made it and tests seem to still pass. Let me know if you need me to make any additional changes.

iamkun · 2021-01-02T11:00:05Z

LGTM. Let get this version released and see if there's any improvement that we could do.

# [1.10.0](v1.9.8...v1.10.0) (2021-01-03) ### Bug Fixes * add ordinal to localeData plugin ([#1266](#1266)) ([fd229fa](fd229fa)) * add preParsePostFormat plugin & update Arabic [ar] locale ([#1255](#1255)) ([f2e4790](f2e4790)) * add type support for plural forms of units ([#1289](#1289)) ([de49bb1](de49bb1)) * escape last period to match only milliseconds ([#1239](#1239)) ([#1295](#1295)) ([64037e6](64037e6)) ### Features * add ES6 Module Support, package.json module point to "esm/index.js" ([#1298](#1298)) ([f63375d](f63375d)), closes [#598](#598) [#313](#313)

iamkun · 2021-01-03T08:27:48Z

🎉 This PR is included in version 1.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [1.10.0](iamkun/dayjs@v1.9.8...v1.10.0) (2021-01-03) ### Bug Fixes * add ordinal to localeData plugin ([#1266](iamkun/dayjs#1266)) ([fd229fa](iamkun/dayjs@fd229fa)) * add preParsePostFormat plugin & update Arabic [ar] locale ([#1255](iamkun/dayjs#1255)) ([f2e4790](iamkun/dayjs@f2e4790)) * add type support for plural forms of units ([#1289](iamkun/dayjs#1289)) ([de49bb1](iamkun/dayjs@de49bb1)) * escape last period to match only milliseconds ([#1239](iamkun/dayjs#1239)) ([#1295](iamkun/dayjs#1295)) ([64037e6](iamkun/dayjs@64037e6)) ### Features * add ES6 Module Support, package.json module point to "esm/index.js" ([#1298](iamkun/dayjs#1298)) ([f63375d](iamkun/dayjs@f63375d)), closes [#598](iamkun/dayjs#598) [#313](iamkun/dayjs#313)

ahmadconnectedmotion

LGTM

# [1.10.0](iamkun/dayjs@v1.9.8...v1.10.0) (2021-01-03) ### Bug Fixes * add ordinal to localeData plugin ([#1266](iamkun/dayjs#1266)) ([fd229fa](iamkun/dayjs@fd229fa)) * add preParsePostFormat plugin & update Arabic [ar] locale ([#1255](iamkun/dayjs#1255)) ([f2e4790](iamkun/dayjs@f2e4790)) * add type support for plural forms of units ([#1289](iamkun/dayjs#1289)) ([de49bb1](iamkun/dayjs@de49bb1)) * escape last period to match only milliseconds ([#1239](iamkun/dayjs#1239)) ([#1295](iamkun/dayjs#1295)) ([64037e6](iamkun/dayjs@64037e6)) ### Features * add ES6 Module Support, package.json module point to "esm/index.js" ([#1298](iamkun/dayjs#1298)) ([f63375d](iamkun/dayjs@f63375d)), closes [#598](iamkun/dayjs#598) [#313](iamkun/dayjs#313)

VehpuS added 5 commits December 4, 2020 22:16

Merge pull request #1 from iamkun/dev

acd8736

Update recent changes in main branch for merging to dev branch

feat: add preparse and postformat features to locales as a plugin

e4f4b02

test: translate moment tests for preparse postformat for the new plugin

0211f07

feat: add preparse postformat to ar locale from moment

e26e802

test: add test for ar locale to ensure preparse and postformat works

a5b9200

iamkun reviewed Dec 31, 2020

View reviewed changes

refactor: change preparse-postformat to require importing localedata …

e565267

…explicitly

iamkun approved these changes Jan 2, 2021

View reviewed changes

iamkun merged commit f2e4790 into iamkun:dev Jan 2, 2021

VehpuS mentioned this pull request Jan 2, 2021

Non-Arabic Numerals #416

Closed

iamkun added the released label Jan 3, 2021

This was referenced Mar 16, 2021

chore(deps): bump dayjs from 1.9.4 to 1.10.4 backstage/backstage#4967

Merged

Bump dayjs from 1.9.7 to 1.10.4 glifio/verifier-frontend#37

Closed

HELWATANY mentioned this pull request Oct 20, 2021

preParsePostFormat documentation is missing dayjs/dayjs-website#60

Closed

ahmadconnectedmotion reviewed Aug 24, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preparse postformat + implementation in ar locale + tests #1255

Preparse postformat + implementation in ar locale + tests #1255

VehpuS commented Dec 6, 2020 •

edited

Loading

codecov bot commented Dec 6, 2020 •

edited

Loading

VehpuS commented Dec 23, 2020

iamkun commented Dec 24, 2020

VehpuS commented Dec 24, 2020 •

edited

Loading

iamkun commented Dec 24, 2020

VehpuS commented Dec 24, 2020

iamkun commented Dec 24, 2020

VehpuS commented Dec 24, 2020

VehpuS commented Dec 24, 2020

iamkun commented Dec 24, 2020

VehpuS commented Dec 24, 2020 •

edited

Loading

iamkun commented Dec 25, 2020

VehpuS commented Dec 30, 2020

iamkun Dec 31, 2020

iamkun commented Dec 31, 2020

VehpuS commented Dec 31, 2020

iamkun commented Jan 1, 2021

VehpuS commented Jan 1, 2021

iamkun commented Jan 1, 2021

VehpuS commented Jan 1, 2021

iamkun commented Jan 2, 2021

iamkun commented Jan 3, 2021

ahmadconnectedmotion left a comment

Preparse postformat + implementation in ar locale + tests #1255

Preparse postformat + implementation in ar locale + tests #1255

Conversation

VehpuS commented Dec 6, 2020 • edited Loading

codecov bot commented Dec 6, 2020 • edited Loading

Codecov Report

VehpuS commented Dec 23, 2020

iamkun commented Dec 24, 2020

VehpuS commented Dec 24, 2020 • edited Loading

iamkun commented Dec 24, 2020

VehpuS commented Dec 24, 2020

iamkun commented Dec 24, 2020

VehpuS commented Dec 24, 2020

VehpuS commented Dec 24, 2020

iamkun commented Dec 24, 2020

VehpuS commented Dec 24, 2020 • edited Loading

iamkun commented Dec 25, 2020

VehpuS commented Dec 30, 2020

iamkun Dec 31, 2020

Choose a reason for hiding this comment

iamkun commented Dec 31, 2020

VehpuS commented Dec 31, 2020

iamkun commented Jan 1, 2021

VehpuS commented Jan 1, 2021

iamkun commented Jan 1, 2021

VehpuS commented Jan 1, 2021

iamkun commented Jan 2, 2021

iamkun commented Jan 3, 2021

ahmadconnectedmotion left a comment

Choose a reason for hiding this comment

VehpuS commented Dec 6, 2020 •

edited

Loading

codecov bot commented Dec 6, 2020 •

edited

Loading

VehpuS commented Dec 24, 2020 •

edited

Loading

VehpuS commented Dec 24, 2020 •

edited

Loading