X Tutup
The Wayback Machine - https://web.archive.org/web/20201008114806/https://github.com/angular/angular/pull/38828
Skip to content

/ angular

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

Jump to bottom

feat(common): Add ISO week-numbering year formats support to formatDate #38828

Closed
wants to merge 1 commit into from
Closed

feat(common): Add ISO week-numbering year formats support to formatDate #38828

wants to merge 1 commit into from
+53 −1

Conversation

@prashanttholia
Copy link
Contributor

@prashanttholia prashanttholia commented Sep 12, 2020

Add ISO 8601 week-numbering year formats ('r', 'rr', 'rrr', 'rrrr') support for formatDate function.

Issue:#38739

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

No current behavior. New feature.

Issue Number: 38739

What is the new behavior?

Examples -
formatDate('2013-06-14', 'rrrr', 'en') //'2013'
formatDate('2013-12-29', 'rrrr', 'en') //'2013'
formatDate('2013-12-31', 'rrrr', 'en') //'2014'

formatDate('2010-08-04', 'rrrr', 'en') //'2010'
formatDate('2010-01-02', 'rrrr', 'en') //'2009'
formatDate('2010-01-04', 'rrrr', 'en') //'2010'

Does this PR introduce a breaking change?

  • Yes
  • No
@googlebot googlebot added the cla: yes label Sep 12, 2020
@pullapprove pullapprove bot requested a review from AndrewKushnir Sep 12, 2020
@prashanttholia prashanttholia force-pushed the prashanttholia:master branch from 413adbb to 4d5a018 Sep 12, 2020
@petebacondarwin petebacondarwin mentioned this pull request Sep 13, 2020
Closed
5 of 14 tasks complete
@petebacondarwin petebacondarwin self-requested a review Sep 13, 2020
@ngbot ngbot bot added this to the needsTriage milestone Sep 13, 2020
@sonukapoor
sonukapoor reviewed Sep 13, 2020
View changes
packages/common/src/i18n/format_date.ts Show resolved Hide resolved
@prashanttholia prashanttholia requested a review from sonukapoor Sep 13, 2020
@AndrewKushnir
AndrewKushnir reviewed Sep 15, 2020
View changes
packages/common/src/i18n/format_date.ts Outdated Show resolved Hide resolved
prashanttholia added a commit to prashanttholia/angular that referenced this pull request Sep 15, 2020
@prashanttholia
feat(common): Update for PR angular#38828 'Add ISO week-numbering yea…
71026ed 
…r formats support to formatDate'

Merged similar code.
@sonukapoor
Copy link
Contributor

@sonukapoor sonukapoor commented Sep 15, 2020

@prashant-tholia Can you also fix the linting issues? The file is not formatted. You can format the file using:

yarn ng-dev format files packages/common/src/i18n/format_date.ts
prashanttholia added a commit to prashanttholia/angular that referenced this pull request Sep 15, 2020
@prashanttholia
feat(common): Formatting fix - PR angular#38828 'Add ISO week-numberi…
0c6ec7c 
…ng year formats support to formatDate'

Fix formatting issues
@prashanttholia
Copy link
Contributor Author

@prashanttholia prashanttholia commented Sep 15, 2020

Done.
@petebacondarwin
petebacondarwin approved these changes Sep 15, 2020
View changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Great work thanks @prashanttholia.

Can you squash all the commits into one?

Normally one does add additional commits to update after a review but they need to be "fixup" commits, which our merge script will automatically squash. But since your commits are not fixups then you might as well do the squashing too.
packages/common/src/i18n/format_date.ts Outdated Show resolved Hide resolved
packages/common/src/i18n/format_date.ts Outdated Show resolved Hide resolved
prashanttholia added a commit to prashanttholia/angular that referenced this pull request Sep 15, 2020
@prashanttholia
feat(common): Formatting Fix - PR angular#38828
389a7ac 
Fix comment formatting
@prashanttholia prashanttholia force-pushed the prashanttholia:master branch from 389a7ac to 6c8af9c Sep 15, 2020
@prashanttholia
Copy link
Contributor Author

@prashanttholia prashanttholia commented Sep 15, 2020

Thanks a lot, @petebacondarwin! I have a query - At present, for 'r' formats, I have passed useISOFormat parameter as true to the getThursdayThisWeek function, making it an ISO 8601 implementation. Wheras, the implementation for the formats 'w' & 'W' is a non-ISO one. This could give results which are in themselves inconsistent for format patterns such as 'rrrr ww'. I request you to let me know whether I should change implementation for 'r' format to use non-ISO as well?

I also wanted to mention that I had worked on ISO implementation of 'w' & 'W' formats as well. So, in case you choose to make the implementation consistent using ISO 8601, I can commit it right away.
@AndrewKushnir
AndrewKushnir approved these changes Sep 15, 2020
View changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Thanks for additional changes @prashanttholia 👍 I've just left a minor comment on removing () around days to Thu calculation expression.
packages/common/src/i18n/format_date.ts Outdated Show resolved Hide resolved
@prashanttholia prashanttholia force-pushed the prashanttholia:master branch from 88f7f34 to c60b015 Sep 15, 2020
@petebacondarwin
petebacondarwin reviewed Sep 17, 2020
View changes
packages/common/src/i18n/format_date.ts Outdated Show resolved Hide resolved
@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Sep 17, 2020

Thanks a lot, @petebacondarwin! I have a query - At present, for 'r' formats, I have passed useISOFormat parameter as true to the getThursdayThisWeek function, making it an ISO 8601 implementation. Wheras, the implementation for the formats 'w' & 'W' is a non-ISO one. This could give results which are in themselves inconsistent for format patterns such as 'rrrr ww'. I request you to let me know whether I should change implementation for 'r' format to use non-ISO as well?

I also wanted to mention that I had worked on ISO implementation of 'w' & 'W' formats as well. So, in case you choose to make the implementation consistent using ISO 8601, I can commit it right away.

Hmm, If I understand correctly the difference is that our current algorithm assumes that the week starts on Sunday, whereas the ISO spec (?) has it starting on Monday. Correct?

I think that perhaps what we should have is a backward compatible consistent mode (i.e. rrrr and ww uses the Thursday with week starting on Sunday). And then we could have an "ISO" mode, that flips all these to the week starting on Monday. Not sure how best that would be exposed.

But in the meantime, I "think" that for consistency we should start with the backward compatible mode for both r and w.
WDYT @AndrewKushnir ?
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Sep 18, 2020

I think that perhaps what we should have is a backward compatible consistent mode (i.e. rrrr and ww uses the Thursday with week starting on Sunday). And then we could have an "ISO" mode, that flips all these to the week starting on Monday. Not sure how best that would be exposed.

I think we may consider introducing a new token (similar to LOCALE_ID) that can control whether ISO format should be used. It can be consumed by i18n date pipe and transform the value based on that.

But in the meantime, I "think" that for consistency we should start with the backward compatible mode for both r and w.

Agree, I think we should have the version that'd be consistent with the current implementation and introduce ISO-based transformation later as a new feature.
prashanttholia added a commit to prashanttholia/angular that referenced this pull request Sep 18, 2020
@prashanttholia
feat(common): PR angular#38828 update - Change week numbering year im…
1715c0d 
…plementation to use non ISO week by default

Week numbering year implementation update
@prashanttholia prashanttholia force-pushed the prashanttholia:master branch 2 times, most recently from ba74534 to a336319 Sep 18, 2020
@prashanttholia
Copy link
Contributor Author

@prashanttholia prashanttholia commented Sep 18, 2020

@petebacondarwin @AndrewKushnir , updated to make it backward compatible and consistent.
@prashanttholia
Copy link
Contributor Author

@prashanttholia prashanttholia commented Sep 18, 2020

Hi, I'm not able to figure out what is causing the check failure. I request for help.
@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Sep 18, 2020

It looks like a couple of ngcc workers crashed, which then probably caused the later failure. I've restarted the job.
@prashanttholia
Copy link
Contributor Author

@prashanttholia prashanttholia commented Sep 18, 2020

Thanks.
@AndrewKushnir
AndrewKushnir requested changes Sep 19, 2020
View changes
packages/common/src/i18n/format_date.ts Outdated Show resolved Hide resolved
@prashanttholia prashanttholia force-pushed the prashanttholia:master branch 2 times, most recently from 2a781cf to a33de4c Sep 19, 2020
@prashanttholia
feat(common): Add ISO week-numbering year formats support to formatDate
Loading status checks…
a891f57 
Add ISO 8601 week-numbering year formats ('r', 'rr', 'rrr', 'rrrr') support for formatDate function.

Issue:#38739
@prashanttholia prashanttholia force-pushed the prashanttholia:master branch from a33de4c to a891f57 Sep 19, 2020
@AndrewKushnir
AndrewKushnir approved these changes Sep 19, 2020
View changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment •
edited

Thanks for the updates @prashanttholia 👍

The changes look good, I've also started tests in Google's codebase and will keep this thread updated.
@prashanttholia
Copy link
Contributor Author

@prashanttholia prashanttholia commented Sep 19, 2020

Thanks, @AndrewKushnir. I'll look forward to the updates.
@AndrewKushnir AndrewKushnir removed the request for review from sonukapoor Sep 21, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Sep 21, 2020

Hi @prashanttholia, the tests in Google's codebase went well and this PR has the necessary approvals now, so I'm adding this PR to the merge queue. Thanks for contributing to Angular framework!
@prashanttholia
Copy link
Contributor Author

@prashanttholia prashanttholia commented Sep 21, 2020

Thanks @AndrewKushnir! I'm glad to know that the PR is being added to the merge queue. Thanks @petebacondarwin for the opportunity to contribute!
@mhevery mhevery closed this in 984ed39 Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petebacondarwin petebacondarwin

@sonukapoor sonukapoor

@AndrewKushnir AndrewKushnir

Assignees
No one assigned
Labels
action: merge cla: yes comp: common comp: i18n feature target: major
Projects
None yet
Milestone
needsTriage
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@prashanttholia @sonukapoor @petebacondarwin @AndrewKushnir @googlebot
You can’t perform that action at this time.
X Tutup