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 parseISODuration #2302

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

benjamincharity
Copy link

Adds a function to parse a string into a duration according to the ISO 8601 format

NOTE: This PR simply adds finishing touches to the awesome work already done by @wuzzeb here: #1947

@benjamincharity
Copy link
Author

benjamincharity commented Mar 9, 2021

@kossnocorp

a) I noticed that when I ran the build script prior to creating my PR, it generated several files that seemed unrelated
b) yarn test passed locally prior to running the build script - possibly failure is due to point a?

Feel free to point me in the direction needed to fix these (this is my first dive into the date-fns repo as a contributor)

Copy link
Member

@kossnocorp kossnocorp left a comment

Choose a reason for hiding this comment

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

Looks good. I cleaned up docs a bit and partially solved the issue with extra files.

@andystalick
Copy link

Could someone re-run these tests? Looks like there is a problem finding an example file.

* ISO 8601: http://en.wikipedia.org/wiki/ISO_8601
*
* If the argument isn't a string, the function cannot parse the string or
* the values are invalid, it returns null.
Copy link

Choose a reason for hiding this comment

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

I came here looking for a function like this, so I'm super excited to see that someone's working on one. One small comment though: the documentation and typings all say this will return null for these exceptional cases, but the actual code returns {}. I like null better, but either way they should match.

Copy link
Author

Choose a reason for hiding this comment

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

Good callout. This is a doc that wasn't updated. I went with {} based on @kossnocorp 's suggestion. I'll get this update pushed today.

@benjamincharity
Copy link
Author

@kossnocorp Whenever you get a chance, these workflows are ready to run again 👍

@Pickachu
Copy link

Howwdy 😄 , thanks for the progress made with this issue! I'm looking forward for the addtion to the of this function to the lib.

Taking a quick look at the source, I see no reference for the week descriptor (W)

var nr = '\\d+(?:[\\.,]\\d+)?'
var dateRegex = '(' + nr + 'Y)?(' + nr + 'M)?(' + nr + 'D)?'
var timeRegex = 'T(' + nr + 'H)?(' + nr + 'M)?(' + nr + 'S)?'
var durationRegex = new RegExp('P' + dateRegex + '(?:' + timeRegex + ')?')

According Luxon specs metioned here and the standard, it seems that this value should be possible:

    check("P1W1DT13H23M34S", { weeks: 1, days: 1, hours: 13, minutes: 23, seconds: 34 });

Was this intentionally left out until the next version of this function?

@adrienharnay
Copy link

Hello, is this still blocked? What is needed to get it to the last mile?

@andystalick
Copy link

Thanks so much for approving the MR! We are real close, just need someone with permissions to resolve these conflicts. ❤️

@fturmel
Copy link
Member

fturmel commented Jan 19, 2022

@benjamincharity need any help to finish this PR? I can pick it up if you want.

At the very least, conflicts should be resolved and the code should be converted to TypeScript.

@fturmel fturmel linked an issue Jan 19, 2022 that may be closed by this pull request
@benjamincharity
Copy link
Author

benjamincharity commented Jan 20, 2022

@fturmel feel free to pick this up. I’m a bit slammed at work currently.

@fturmel fturmel marked this pull request as draft January 20, 2022 01:27
@fturmel
Copy link
Member

fturmel commented Jan 20, 2022

@benjamincharity I rebased and converted to TS, but if that's OK I'll need write permission to your branch so we can keep all the work here in the same PR. If not, let me know how you want to proceed.

I also found a bug and some opportunities to improve the PR, so I've put this in draft mode for now.

@benjamincharity
Copy link
Author

@fturmel I've added you as a collaborator

@its-danny
Copy link

any updates on this?

@sslgeorge
Copy link

@fturmel can you please share more information about the bug you found, someone else could jump in and help if you are slammed with work.

@its-danny
Copy link

for anyone needing this, i've moved to using duration-fns for now and it works.

@davetapley
Copy link

Third attempt at getting this in! #3151

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.

How to convert a "PT12H" value into Time