Add fromISOTime, toISOTime and toMillis to Duration #803

thomas-darling · 2020-10-06T14:12:53Z

This resolves #625, by adding a fromISOTime and toISOTime method to the Duration class.
The design of the methods is closely aligned with the design of similar, existing methods, such as fromISO and toISO.

Additionally, since the Duration class has a fromMillis method, I've also added a corresponding toMillis method.
The justification for this is, that having corresponding pairs of from/to methods, makes it easy to roundtrip a value in the same format, and that it aligns with the DateTime class, which already has both method.

jsf-clabot · 2020-10-06T14:12:55Z

All committers have signed the CLA.

icambron · 2020-10-21T05:01:44Z

src/duration.js

+      format = format.replace(/:/g, "");
+    }
+
+    if (opts.suppressPrefix === "never" || opts.suppressPrefix === "auto" && (format === "hh" || format === "hhmm")) {


I'm not against options like these, but we need to make sure they're consistent. We don't support these elsewhere, do we?

Yep, DateTime has similar options, so I designed this to behave the same way.
See https://github.com/moment/luxon/blob/master/src/datetime.js#L1534

Specifically regarding the prefix suppression - this is unique to the time format:

As of ISO 8601-1:2019, the basic format is T[hh][mm][ss] and the extended format is T[hh]:[mm]:[ss]. Earlier versions omitted the T in both formats.
...
ISO 8601-1:2019 allows the T to be omitted in the extended format, as in "13:47:30", but only allows the T to be omitted in the basic format when there is no risk of ambiguity with date expressions.

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

So I made the default behavior here auto, i.e. omit if possible - but if you're interacting with a system that always/never expects the prefix, this option allows you to override that behavior.

icambron · 2020-10-21T05:04:14Z

src/impl/regexParser.js

@@ -118,6 +118,29 @@ function extractIANAZone(match, cursor) {
  return [{}, zone, cursor + 1];
 }

+// ISO time parsing
+
+const isoTimeOnly = /^T?(\d\d)(?::?(\d\d)(?::?(\d\d)(?:[.,](\d{1,30}))?)?)?$/; 


Aren't these identical to isoTimeBaseRegex and extractISOTime?

They are very similar, but not quite identical.

isoTimeOnly includes the optional T prefix, and matches the beginning/end of the string - isoTimeBaseRegex does not.

This means isoTimeBaseRegex would match a value such as foo00bar, even though that's obviously not a valid Time value.
But I guess we could consider changing it to this, to reuse the existing regex:

const isoTimeOnly = RegExp(`^T?${isoTimeBaseRegex.source}$`)

extractISOTimeOnly preserves the precision of the value, by only setting the values that are actually specified in the string - isoTimeBaseRegex always sets e.g. seconds to 0, even if not specified.

Either the seconds, or the minutes and seconds, may be omitted from the basic or extended time formats for greater brevity but decreased precision - https://en.wikipedia.org/wiki/ISO_8601#Times

One could of course argue extractISOTime should be fixed so it too preserves the precision - I'm just afraid that might be a breaking change, as code may have been written to expect seconds in a DateTime instance to always have a value.

There's also a minor difference in the property names in the returned object - e.g. seconds instead of second. That's because Duration.fromObject only accepts the plural version, which kinda makes sense, as it may represent arbitrary durations, and the singular name only makes sense when talking specifically about a time of day.

thomas-darling · 2020-10-21T15:28:05Z

Hmm, looking at this again, I'd actually like to refactor it a little.
The way I implemented the options here, is not quite as consistent with the rest of the code as I thought it was 🙂

icambron · 2020-11-09T15:25:20Z

@thomas-darling ok, let me know when you want me to take another look. I can spend some real time looking at this when it's ready; my comments so far were the result of only a very quick readthrough

thomas-darling · 2020-11-09T15:31:25Z

@icambron, will do 👍
I got a little busy here, but I'm working on it, so should have it ready for review soon 🙂

thomas-darling · 2020-11-10T23:15:43Z

test/datetime/format.test.js

-test("DateTime#toISOTime({suppressSeconds: true}) will suppress milliseconds if they're zero", () => {
-  expect(dt.set({ second: 0, millisecond: 0 }).toISOTime({ suppressSeconds: true })).toBe("09:23Z");
-});
-


Removed because it is identical to the test above

thomas-darling · 2020-11-11T00:58:08Z

test/duration/parse.test.js

+  checkTime("11", { hours: 11, minutes: 0, seconds: 0 });
+  checkTime("T1122", { hours: 11, minutes: 22, seconds: 0 });
+});
+


This exactly matches how the time component of a date is parsed - including the, in my opinion, strange behavior where milliseconds are undefined if not specified, while the other values default to 0.

A general discussion could be had about whether defaulting to 0 is the correct behavior, as it means we actually loose the information about the precision of the value. That said, the need for that information is probably an uncommon edge case, and defaulting to 0 is very convenient, as it means users won't need null checks - so I'm ok with that.

However, I do think milliseconds should then also default to 0, both here and when parsing a date, as the current behavior is quite unexpected - especially since the typings for DateTime claim the millisecond property will always be a number...

Thoughts?

thomas-darling · 2020-11-11T01:21:34Z

src/duration.js

+
+    return value.toFormat(fmt);
+  }
+


This method is designed to behave just like the existing toISOTime() in the DateTime class - and that one has been extended to support adding the prefix, but is otherwise unchanged.

In my first implementation, the default was to automatically choose whether to add the prefix, depending on the format. However, I've since decided to drop that, and instead make it an explicit option, defaulting to false.

Technically, the 2019 version of the spec does require the prefix to be present for the basic format hhmm, but the previous version didn't, so automatically adding it now would have been a breaking change.

Also, since the user already has to specify options to get the basic format, they might as well also specify if they want the prefix...

thomas-darling · 2020-11-11T01:32:00Z

@icambron I think this is done now, so feel free to review when you have the time 🙂

~~(by the way, I think the CI build may be stuck - looks like it's been queued for quite a while now)~~
Never mind, it works now

thomas-darling · 2020-11-11T13:04:44Z

@icambron, turns out I had a few bugs there - but should be all done now 🙂

JensUngerer · 2020-12-06T08:04:55Z

I've justed "toied with the idea of doing" a merge request for a toMillis method in the Duration class... .
I am happy that i found this merge - request a minute ago...

I am using luxon for my TimeTracker project.
It is exactly what i've searched for.
"But", it "lacked" one kind of "functionality": a toMillis method in the Duration class... .

@icambron, turns out I had a few bugs there - but should be all done now 🙂

I am looking forward seeing the merge - request to be merged.

icambron · 2021-02-13T18:42:55Z

Sorry for the long delay. Merged and will be released shortly. Thanks!

Add fromISOTime, toISOTime and toMillis to Duration

fc1bcf9

Add tests for fromISOTime, toISOTime and toMillis in Duration

1e9117a

icambron reviewed Oct 21, 2020

View reviewed changes

Reuse existing isoTimeBaseRegex

bedebb4

icambron added the PR: needs work label Nov 9, 2020

thomas-darling added 3 commits November 10, 2020 23:08

Refactored toISOTime

96434ce

Simplified toISOString

2cb632e

Use existing extractISOTime in parseISOTimeOnly

ab4c2b1

thomas-darling commented Nov 10, 2020

View reviewed changes

thomas-darling added 3 commits November 11, 2020 01:04

Fix tests for fromISOTime and toISOTime

690b955

Remove unnessesary tests

3b7b6cc

Fix comments

8b8ad62

thomas-darling commented Nov 11, 2020

View reviewed changes

thomas-darling added 5 commits November 11, 2020 02:50

Fixed bugs and lint issues

dd611c7

Fix bug, and make the piece of shit that is Prettier shut up

6e568cd

Fix failing tests

98e16c7

Fixed bugs and lint issues

7099d38

Fix failing tests

0a12a42

icambron merged commit 8ec10bb into moment:master Feb 13, 2021

dependabot bot mentioned this pull request Mar 11, 2021

Bump luxon from 1.25.0 to 1.26.0 umcconnell/umcconnell.github.io#1

Closed

dependabot bot mentioned this pull request Mar 16, 2021

Bump luxon from 1.25.0 to 1.26.0 in /website Korbeil/phpstan#10

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fromISOTime, toISOTime and toMillis to Duration #803

Add fromISOTime, toISOTime and toMillis to Duration #803

thomas-darling commented Oct 6, 2020

jsf-clabot commented Oct 6, 2020 •

edited

icambron Oct 21, 2020

thomas-darling Oct 21, 2020

thomas-darling Oct 21, 2020

icambron Oct 21, 2020

thomas-darling Oct 21, 2020 •

edited

thomas-darling commented Oct 21, 2020

icambron commented Nov 9, 2020

thomas-darling commented Nov 9, 2020

thomas-darling Nov 10, 2020

thomas-darling Nov 11, 2020 •

edited

thomas-darling Nov 11, 2020 •

edited

thomas-darling commented Nov 11, 2020 •

edited

thomas-darling commented Nov 11, 2020

JensUngerer commented Dec 6, 2020 •

edited

icambron commented Feb 13, 2021

Add fromISOTime, toISOTime and toMillis to Duration #803

Add fromISOTime, toISOTime and toMillis to Duration #803

Conversation

thomas-darling commented Oct 6, 2020

jsf-clabot commented Oct 6, 2020 • edited

icambron Oct 21, 2020

Choose a reason for hiding this comment

thomas-darling Oct 21, 2020

Choose a reason for hiding this comment

thomas-darling Oct 21, 2020

Choose a reason for hiding this comment

icambron Oct 21, 2020

Choose a reason for hiding this comment

thomas-darling Oct 21, 2020 • edited

Choose a reason for hiding this comment

thomas-darling commented Oct 21, 2020

icambron commented Nov 9, 2020

thomas-darling commented Nov 9, 2020

thomas-darling Nov 10, 2020

Choose a reason for hiding this comment

thomas-darling Nov 11, 2020 • edited

Choose a reason for hiding this comment

thomas-darling Nov 11, 2020 • edited

Choose a reason for hiding this comment

thomas-darling commented Nov 11, 2020 • edited

thomas-darling commented Nov 11, 2020

JensUngerer commented Dec 6, 2020 • edited

icambron commented Feb 13, 2021

jsf-clabot commented Oct 6, 2020 •

edited

thomas-darling Oct 21, 2020 •

edited

thomas-darling Nov 11, 2020 •

edited

thomas-darling Nov 11, 2020 •

edited

thomas-darling commented Nov 11, 2020 •

edited

JensUngerer commented Dec 6, 2020 •

edited