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

Duration from ISO leading minus #683

Merged
merged 2 commits into from
Apr 2, 2020

Conversation

jmadelaine
Copy link
Contributor

This PR fixes moment/luxon#681.

The issue above links to a similar issue with moment (moment/moment#2408) which was resolved.

Although the ISO 8601 duration format does not mention support for a leading minus, there are many other libraries that support ISO duration parsing in this way.

The assumed rule is that a leading minus sign inverts the sign of all values.

Examples of leading-minus ISO strings and their equivalent non-leading-minus ISO strings:

-P1D == P-1D
-P5Y3M == P-5Y-3M
-P-5Y-3M == P5Y3M
-P-1W1DT13H-23M34S == P1W-1DT-13H23M-34S

@jsf-clabot
Copy link

jsf-clabot commented Apr 2, 2020

CLA assistant check
All committers have signed the CLA.

package.json Outdated Show resolved Hide resolved
src/impl/util.js Outdated Show resolved Hide resolved
… on negative prefix

We check that num is not undefined before negation, as returning NaN from extractISODuration throws an error inside asNumber util function.
@jmadelaine jmadelaine force-pushed the duration-from-iso-leading-minus branch from 5b12f33 to 66ed38d Compare April 2, 2020 20:02
@icambron
Copy link
Member

icambron commented Apr 2, 2020

Looks good, thanks! I'll get this released very soon.

@icambron icambron merged commit 17832ea into moment:master Apr 2, 2020
@jmadelaine jmadelaine deleted the duration-from-iso-leading-minus branch April 2, 2020 21:05
@icambron
Copy link
Member

icambron commented Apr 2, 2020

Released as 1.23.0

vipulnsward added a commit to vipulnsward/rails that referenced this pull request Jun 4, 2020
…TS(ex: moment/luxon#683, moment/moment#2408), many do not.

For example PG refers to https://www.ietf.org/rfc/rfc3339.txt when converting(Ref: https://www.postgresql.org/docs/current/datatype-datetime.html)

According to the ref there is no explicit mention of allowing sign before the parts, which reads as below:

 Durations:

    dur-second        = 1*DIGIT "S"
    dur-minute        = 1*DIGIT "M" [dur-second]
    dur-hour          = 1*DIGIT "H" [dur-minute]
    dur-time          = "T" (dur-hour / dur-minute / dur-second)
    dur-day           = 1*DIGIT "D"
    dur-week          = 1*DIGIT "W"
    dur-month         = 1*DIGIT "M" [dur-day]
    dur-year          = 1*DIGIT "Y" [dur-month]
    dur-date          = (dur-day / dur-month / dur-year) [dur-time]

    duration          = "P" (dur-date / dur-time / dur-week)

We should not attempt to move sign forward in this case.
rafaelfranca pushed a commit to rails/rails that referenced this pull request Nov 2, 2020
…TS(ex: moment/luxon#683, moment/moment#2408), many do not.

For example PG refers to https://www.ietf.org/rfc/rfc3339.txt when converting(Ref: https://www.postgresql.org/docs/current/datatype-datetime.html)

According to the ref there is no explicit mention of allowing sign before the parts, which reads as below:

 Durations:

    dur-second        = 1*DIGIT "S"
    dur-minute        = 1*DIGIT "M" [dur-second]
    dur-hour          = 1*DIGIT "H" [dur-minute]
    dur-time          = "T" (dur-hour / dur-minute / dur-second)
    dur-day           = 1*DIGIT "D"
    dur-week          = 1*DIGIT "W"
    dur-month         = 1*DIGIT "M" [dur-day]
    dur-year          = 1*DIGIT "Y" [dur-month]
    dur-date          = (dur-day / dur-month / dur-year) [dur-time]

    duration          = "P" (dur-date / dur-time / dur-week)

We should not attempt to move sign forward in this case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duration.fromISO cant handle leading minus sign
3 participants