Luxon: Duration#toISO() may not conform to ISO 8601.

[munierujp]

Weeks should be expressed as days, isn't it?

https://en.wikipedia.org/wiki/ISO_8601#Durations

Example

const duration = Duration.fromObject({
  years: 3,
  months: 6,
  weeks: 1,
  days: 4,
  hours: 12,
  minutes: 30,
  seconds: 5
})
console.log(duration.toISO())

• expected: P3Y6M11DT12H30M5S
  
  
  
  • 1[weeks] + 4[days] = 7[days] + 4[days] = 11[days]
  
  
• actual: P3Y6M1W4DT12H30M5S

FYI

in Java's Period#parse():

ISO-8601 does not permit mixing between the PnYnMnD and PnW formats. Any week-based input is multiplied by 7 and treated as a number of days.

https://docs.oracle.com/en/java/javase/12/docs/api/java.base/java/time/Period.html#parse(java.lang.CharSequence)

[icambron]

well, it should certainly be compliant with the spec. However, before I make that change, I'd like to see the relevant language in the ISO spec, which I don't have access to

[munierujp]

It seems that we need to buy to see it... 💸

https://www.iso.org/standard/70908.html

[leebickmtu]

I was able to find a draft of the spec prior to finalization in PDF format.
http://www.loc.gov/standards/datetime/iso-tc154-wg5_n0038_iso_wd_8601-1_2016-02-16.pdf

Relevant section is 4.4.3.2 on page 23. So long as the final spec did not change the format is specified as:

In both basic and extended format the complete representation of the expression for duration shall be [PnnW] or [PnnYnnMnnDTnnHnnMnnS].

[SneakyFish5]

I would like to work on this but I am kind of confused on what is wanted here with the formats. Should it be like the Java parser and if a obj.weeks is specified just multiply it by 7 and it to days? Or do you still want to keep weeks as part of the ISO output?

[leebickmtu]

Same as java, multiply by 7 and add to days.

Optionally, if only weeks is populated then toISO() could return in [PnnW] format as well. But it is still valid to do [PnnYnnMnnDTnnHnnMnnS] with this case as well and convert to days.

[SneakyFish5]

How does this look to you?

https://github.com/SneakyFish5/luxon/commit/b218512f717940fc9e48f4a09ce5be55d8d0b2dc

Sidenote: This doesn't include docs, I'll get on that though if you think this commit is good or needs some work. I know the first if statement to see if weeks is standalone is pretty long and it autoformatted that way, I'm open to suggestions if there is a better way to do it.

[munierujp]

I think toISO method should always return general format (like P7D).
And we can add other method such as toISOWeeks for weeks format (like P1W).

[leebickmtu]

I don't think a separate toISOWeeks function makes sense, since most of the time it wouldn't be valid. Only when the current unit break down only has weeks in it.

What about a Boolean parameter on toISO which enables it? But still only if just weeks is defined.

Or just don't output that format at all. Spec would require parsing it on input but output isn't really needed. The longer format with weeks multiplied out to days represents it just fine.

[munierujp]

@leebickmtu

What about a second Boolean parameter on toISO which enables it? But still only if just weeks is defined.

It's nice idea 😄

[munierujp]

It would be better if two format names were clearly defined in ISO 8601...🤔

[icambron]

I think it's a little awkward that we only get the W representation if the duration data is just so. Will only serve to surprise people that the strings look really different in some situations. So let's have separate methods:

• toISO always converts weeks to days
• toISOWeeks converts the duration to weeks via as('weeks') and then just formats in that number of weeks

[dan-j]

Has behaviour around this issue changed? I've just arrived here after trying the example from the docs which results in an invalid duration, but the docs suggests it should work.

https://moment.github.io/luxon/docs/class/src/duration.js~Duration.html

Example:

Duration.fromISO('P3Y6M1W4DT12H30M5S').toObject() //=> { years: 3, months: 6, weeks: 1, days: 4, hours: 12, minutes: 30, seconds: 5 }

Actual output: {}

The underlying Duration instance has this invalid property:

{
     reason: 'unparsable',
     explanation: 'the input "P3Y6M1W4DT12H30M5S" can\'t be parsed as ISO 8601'
}

[icambron]

@dan-j it works for me...

[arusak]

@icambron ISO8601 states that numbers in interval string must be positive integers. as('weeks') gives us a fractional value in general. I think we should truncate the value. P156.0000744047619W looks crazy.

Negative durations are out of scope of 8601.

[MikaelLambert]

I've also ran into this issue while ironically converting between momentjs and luxon (2 separate projects) and momentjs does the PnnD conversion as per the spec. I would argue for consistency that luxon match momentjs at least for now and consider implementing the PnnW spec as the alternative, and not the default.

The workaround for me at least is to use luxon.Duration.fromObject({ days: 7 }).toISO()

moment.duration(1, 'weeks').toISOString()
>>"P7D"

moment.duration(7, 'days').toISOString()
>>"P7D"

luxon.Duration.fromObject({ weeks: 1 }).toISO()
>>"P1W"

luxon.Duration.fromObject({ days: 7 }).toISO()
>>"P7D"

https://momentjs.com/docs/#/durations/