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

Add missing padding to 'YYYY' format #2231

Merged
merged 2 commits into from Feb 13, 2023
Merged

Add missing padding to 'YYYY' format #2231

merged 2 commits into from Feb 13, 2023

Conversation

Sharparam
Copy link
Contributor

@Sharparam Sharparam commented Feb 7, 2023

Adds padding to the year component when formatted with 'YYYY', similar to how padding is handled for 'MM' and 'DD'.

Fixes #2230

Adds padding to the year component when formatted with `'YYYY'`, similar to how padding is handled for '`MM`' and '`DD`'.

Fixes iamkun#2230
@iamkun
Copy link
Owner

iamkun commented Feb 11, 2023

Thanks, can you add some relevant unit test,s please

@iamkun iamkun added the next release Will merge into next release label Feb 11, 2023
@codecov
Copy link

codecov bot commented Feb 11, 2023

Codecov Report

Merging #2231 (cc7d38a) into dev (7151139) will not change coverage.
The diff coverage is 100.00%.

❗ Current head cc7d38a differs from pull request most recent head ea97f4c. Consider uploading reports for the commit ea97f4c to get more accurate results

@@            Coverage Diff            @@
##               dev     #2231   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          181       183    +2     
  Lines         2074      2111   +37     
  Branches       544       555   +11     
=========================================
+ Hits          2074      2111   +37     
Impacted Files Coverage Δ
src/index.js 100.00% <ø> (ø)
src/locale/fa.js 100.00% <ø> (ø)
src/locale/en.js 100.00% <100.00%> (ø)
src/locale/fr.js 100.00% <100.00%> (ø)
src/locale/nl.js 100.00% <100.00%> (ø)
src/locale/zh-tw.js 100.00% <100.00%> (ø)
src/plugin/advancedFormat/index.js 100.00% <100.00%> (ø)
src/plugin/bigIntSupport/index.js 100.00% <100.00%> (ø)
src/plugin/duration/index.js 100.00% <100.00%> (ø)
src/plugin/objectSupport/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.

@Sharparam
Copy link
Contributor Author

Added two tests: one to explicitly check that year 1 gets padded with zeroes on formatting, and one test to make sure the formatting matches how moment does it.

@iamkun iamkun merged commit 00c223b into iamkun:dev Feb 13, 2023
@iamkun
Copy link
Owner

iamkun commented Feb 13, 2023

THX

@Sharparam Sharparam deleted the patch-1 branch February 13, 2023 08:53
aleksandr-senichev pushed a commit to aleksandr-senichev/dayjs that referenced this pull request Mar 3, 2023
github-actions bot pushed a commit that referenced this pull request Jun 2, 2023
## [1.11.8](v1.11.7...v1.11.8) (2023-06-02)

### Bug Fixes

* .format add padding to 'YYYY' ([#2231](#2231)) ([00c223b](00c223b))
* Added .valueOf method to Duration class ([#2226](#2226)) ([9b4fcfd](9b4fcfd))
* timezone type mark `date` parameter as optional ([#2222](#2222)) ([b87aa0e](b87aa0e))
* type file first parameter date is optional in isSame(), isBefore(), isAfter() ([#2272](#2272)) ([4d56f3e](4d56f3e))
@github-actions
Copy link

github-actions bot commented Jun 2, 2023

🎉 This PR is included in version 1.11.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

@bensaufley
Copy link

Should there be an option for maybe just Y then, which isn't zero-padded? FWIW I'm not sure that this is consistent with month and day, because MMMM is full english month, not zero-padded month and there is no DDDD.

@Sharparam
Copy link
Contributor Author

@bensaufley I'm not sure what datetime libs you're familiar with, but this behaviour is consistent with how all major libraries behave, including JavaScript itself. YYYY is the full (padded) year, and similar with MM and DD for month and day (or sometimes yyyy, MM and dd with lowercase mm being used for 0-padded minutes), the reason being that year is 4 digits while month and day are 2 digits.

@bensaufley
Copy link

All fair points, but:

  • The JavaScript spec you've listed doesn't have other formats like dayjs does. YYYY represents the year as four digits, and there is no YY or any other year format, nor is there MMM etc. They've just chosen to use YYYY and MM because those are always that fixed length. In fact the spec you're referring to here doesn't actually ever interpret those strings (that is, there is no native .format('YYYY-MM-DD') or similar; that's the whole point of this library)—the docs are just using those strings as a stand-in to describe the valid formats of actual date strings.
  • This change means there's no way for the library to represent the string January 12, 301 or something like that.

I absolutely agree that YYYY-MM-DD as a format is more common and useful, FWIW, and that there should be a way to do it (and maybe this is the way). This just seems like a breaking change and one that may not be internally consistent.

@Sharparam
Copy link
Contributor Author

It's true that it's technically a breaking change. However, given that it is a bug fix for unexpected behaviour (see below note about the docs), I think it being part of a patch bump is reasonable.

It could be useful to add YYY, YY, and Y as ways to get years with different padded lengths if that's a needed feature.

YY already works to get a two-digit year, same as in moment.

While moment does not have YYY, there is some precedent for it since .NET does have it in their formatting (archive link just in case since Microsoft enjoys breaking links sometimes). It should be rather easy to add to dayjs if you're up for it!

It's also worth noting that the docs for dayjs has claimed YYYY to generate 4-digit years since way before this change. Indicating that it not generating 4-digit years was a bug rather than feature.

(I'm also willing to bet that a majority of users would expect YYYY to generate 4 digits, and most people just don't notice otherwise since dates before year 1900, let alone 1000, very rarely occur in actual usage. In a project at work we only noticed dayjs exhibiting the previous behaviour when some min-value dates escaped from the backend (0001-01-01) or as dayjs rather confusingly rendered them then: 1-01-01.)

@bensaufley
Copy link

All sounds good to me and I agree with most or all of your points. I don't have a strong preference but I do think it would probably be good, if probably an edge case (in the same way that people probably mostly didn't encounter this issue), to support non-padded full year in one way or another.

ohsory1324 pushed a commit to ohsory1324/dayjs that referenced this pull request Dec 20, 2023
ohsory1324 pushed a commit to ohsory1324/dayjs that referenced this pull request Dec 20, 2023
## [1.11.8](iamkun/dayjs@v1.11.7...v1.11.8) (2023-06-02)

### Bug Fixes

* .format add padding to 'YYYY' ([iamkun#2231](iamkun#2231)) ([00c223b](iamkun@00c223b))
* Added .valueOf method to Duration class ([iamkun#2226](iamkun#2226)) ([9b4fcfd](iamkun@9b4fcfd))
* timezone type mark `date` parameter as optional ([iamkun#2222](iamkun#2222)) ([b87aa0e](iamkun@b87aa0e))
* type file first parameter date is optional in isSame(), isBefore(), isAfter() ([iamkun#2272](iamkun#2272)) ([4d56f3e](iamkun@4d56f3e))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release Will merge into next release released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'YYYY' results in missing leading zeroes when year <1000
3 participants