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 #1947

Closed
wants to merge 2 commits into from
Closed

Add parseISODuration #1947

wants to merge 2 commits into from

Conversation

wuzzeb
Copy link

@wuzzeb wuzzeb commented Sep 15, 2020

Add function to parse an ISO 8601 duration from a string into the Duration type.

Adds a function to parse a string into a
duration according to the ISO 8601 format
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.

A great addition, thank you! Please return an empty object instead of null for consistency's sake and fix a small typo in the docs, and it will be ready to go.

*
* @example
* // Convert string 'P1DT5M30S' to duration:
* var result = parseISO('P1HT5M30S')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* var result = parseISO('P1HT5M30S')
* var result = parseISODuration('P1HT5M30S')

Object.prototype.toString.call(argument) === '[object String]'
)
) {
return null
Copy link
Member

Choose a reason for hiding this comment

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

Here and everywhere below, I suggest returning an empty object instead of null. It will be more or less consistent with parse, which in similar cases returns Invalid Date or parseInt that return NaN. In all these cases, the returned value is of the expected type. I know that null is "an object," but that's just a bug.

Object.prototype.toString.call(argument) === '[object String]'
)
) {
return null
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return null
return {}


var match = argument.match(durationRegex)
if (!match) {
return null
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return null
return {}

!match[5] &&
!match[6]
) {
return null
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return null
return {}

@wuzzeb
Copy link
Author

wuzzeb commented Sep 18, 2020

The main issue with an empty object is if I want to determine if the entered string is not a valid ISO 8601 duration to say display an error message, it is hard to check if the return value is an empty object while it is easy to check for NaN or Invalid Date.

I guess lodash has the isEmpty function which does the check, and I am using lodash so I guess it isn't a big deal, I can use lodash to check for empty object and display an error.

@kossnocorp
Copy link
Member

@wuzzeb I understand the issue, but one of the library core values is consistency with ECMAScript. And when the user input is processed, we always get a value of the same type so the code could continue work and output something instead of throwing an exception. Also, as you said, you can either check the result for emptiness (Object.keys(result).length will do the trick) or even verify the sum of values (unless you want to allow 0 duration).

@vespertilian
Copy link

Hey guys nice work. I was just looking for this feature and was looking to helpout. Before I found this PR I had gone ahead and pulled the equivalent function out of luxon.

Luxon has some good assertions you might want to copy, such as tests for negative numbers and fractions of a second.

I can add the assertions if you like, just let me know.

Thanks again!

Here is the luxon implementation I copied just for references:

Luxon implementation

const isoDuration = /^-?P(?:(?:(-?\d{1,9})Y)?(?:(-?\d{1,9})M)?(?:(-?\d{1,9})W)?(?:(-?\d{1,9})D)?(?:T(?:(-?\d{1,9})H)?(?:(-?\d{1,9})M)?(?:(-?\d{1,20})(?:[.,](-?\d{1,9}))?S)?)?)$/;

function parse(s, ...patterns) {
  if (s == null) {
    return [null, null];
  }

  for (const [regex, extractor] of patterns) {
    const m = regex.exec(s);
    if (m) {
      return extractor(m);
    }
  }
  return [null, null];
}

function isUndefined(o) {
  return typeof o === "undefined";
}

function parseInteger(string) {
  if (isUndefined(string) || string === null || string === "") {
    return undefined;
  } else {
    return parseInt(string, 10);
  }
}

function parseMillis(fraction) {
  // Return undefined (instead of 0) in these cases, where fraction is not set
  if (isUndefined(fraction) || fraction === null || fraction === "") {
    return undefined;
  } else {
    const f = parseFloat("0." + fraction) * 1000;
    return Math.floor(f);
  }
}

function extractISODuration(match) {
  const [
    s,
    yearStr,
    monthStr,
    weekStr,
    dayStr,
    hourStr,
    minuteStr,
    secondStr,
    millisecondsStr
  ] = match;

  const hasNegativePrefix = s[0] === "-";
  const maybeNegate = num => (num && hasNegativePrefix ? -num : num);

  return [
    Object.assign(
      {},
      yearStr ? {years: maybeNegate(parseInteger(yearStr))} : null,
      monthStr ? {months: maybeNegate(parseInteger(monthStr))} : null,
      weekStr ? {weeks: maybeNegate(parseInteger(weekStr))} : null,
      dayStr ? { days: maybeNegate(parseInteger(dayStr))} : null,
      hourStr ? {hours: maybeNegate(parseInteger(hourStr))} : null,
      minuteStr ? {minutes: maybeNegate(parseInteger(minuteStr))} : null,
      secondStr ? {seconds: maybeNegate(parseInteger(secondStr))} : null,
      millisecondsStr ? {milliseconds: maybeNegate(parseMillis(millisecondsStr))} : null
    )
  ];
}

export function parseISODuration(s) {
  const [parsed] = parse(s, [isoDuration, extractISODuration])
  return parsed ? parsed : {}
}

Luxon specs

import { parseISODuration } from "./parse-iso-duration";

function check(s, ob) {
  expect(parseISODuration(s)).toEqual(ob)
}

describe('parseISODuration', () => {
  it('handles a wide variety of formats', () => {
    check("P5Y3M", { years: 5, months: 3 });
    check("PT54M32S", { minutes: 54, seconds: 32 });
    check("P3DT54M32S", { days: 3, minutes: 54, seconds: 32 });
    check("P1YT34000S", { years: 1, seconds: 34000 });
    check("P1W1DT13H23M34S", { weeks: 1, days: 1, hours: 13, minutes: 23, seconds: 34 });
    check("P2W", { weeks: 2 });
    check("PT10000000000000000000.999S", { seconds: 10000000000000000000, milliseconds: 999 });
  })

  it('parses mixed or negative durations', () => {
    check("P-5Y-3M", { years: -5, months: -3 });
    check("PT-54M32S", { minutes: -54, seconds: 32 });
    check("P-3DT54M-32S", { days: -3, minutes: 54, seconds: -32 });
    check("P1YT-34000S", { years: 1, seconds: -34000 });
    check("P-1W1DT13H23M34S", { weeks: -1, days: 1, hours: 13, minutes: 23, seconds: 34 });
    check("P-2W", { weeks: -2 });
    check("-P1D", { days: -1 });
    check("-P5Y3M", { years: -5, months: -3 });
    check("-P-5Y-3M", { years: 5, months: 3 });
    check("-P-1W1DT13H-23M34S", { weeks: 1, days: -1, hours: -13, minutes: 23, seconds: -34 });
  })

  it('parses fractions of a second', () => {
    check("PT54M32.5S", {minutes: 54, seconds: 32, milliseconds: 500})
    check("PT54M32.53S", {minutes: 54, seconds: 32, milliseconds: 530})
    check("PT54M32.534S", {minutes: 54, seconds: 32, milliseconds: 534})
    check("PT54M32.5348S", {minutes: 54, seconds: 32, milliseconds: 534})
    check("PT54M32.034S", {minutes: 54, seconds: 32, milliseconds: 34})
  })

  it('returns empty object for junk', () => {
    expect(parseISODuration("foo")).toEqual({})
    expect(parseISODuration("PTglorb")).toEqual({})
    expect(parseISODuration("5Y")).toEqual({})
    expect(parseISODuration("P34K")).toEqual({})
    expect(parseISODuration("P5D2W")).toEqual({})
  })
})

@adrienharnay
Copy link

Hey, was just looking for this! Any update? Thanks a lot!

@PandaaAgency
Copy link

Hi, any update about this pull request ?

@benjamincharity
Copy link

@wuzzeb are you able to get the requested changes in? We just hit this and are temporarily using your code directly. I don't want to step on any toes, but am raising my hand 🙋 in case I can assist in getting this across the finish line. Thanks for getting this started!

@kossnocorp
Copy link
Member

@benjamincharity, feel free to finish this. I would also love to hear your thoughts about null vs. {} as you have a use case for the function. Appreciate it!

@chrisfrancis27
Copy link

Just to add my own £0.02, I agree with returning {} rather than null for invalid input. In my mind, the most semantic API would be either:

  • return {} (caller is guaranteed to receive the same return type regardless of input as @kossnocorp suggested, a la new Date('garbage')); or
  • throw Error (caller is explicitly forced to handle the exception thrown from attempting to set an invalid Duration, a la JSON.parse('garbage'))

However, throw feels a little nuclear to me, for something that's essentially a nice-to-have utility.

@benjamincharity
Copy link

@kossnocorp I definitely understand wanting null for an easy check by the consumer; but also realize the value of a consistent API. In this case I would defer to members of this org.

I have pulled in the commits from @wuzzeb and addressed the comments above in the new PR here: #2302

@kossnocorp
Copy link
Member

kossnocorp commented Mar 18, 2021

Thanks, @wuzzeb for the initial development, @benjamincharity continued the PR there: #2302

@kossnocorp kossnocorp closed this Mar 18, 2021
@davetapley
Copy link

Third attempt at getting this in! #3151

@jaxor24
Copy link

jaxor24 commented Dec 2, 2023

Would love this feature.

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.

None yet

9 participants