tolu / ISO8601-duration

Node/Js-module for parsing and making sense of ISO8601-durations
92 stars 10 forks source link

Allowing optional duration values in toSeconds #18

Closed ZigaStrgar closed 3 years ago

ZigaStrgar commented 4 years ago

Motivation

Typescript declaration mentions that each of Durations interface properties is optional but the case in the code it's not the same. Thus not allowing you to use toSeconds as a standalone function without all of the properties present in the object.

Before change

toSeconds({ minutes: 3 }); // NaN
toSeconds({ minutes: 3, years: 0, months: 0, days: 0, hours: 0, seconds: 0, weeks: 0 }); // 180

After change

toSeconds({ minutes: 3 }); // 180
toSeconds({ minutes: 3, years: 0, months: 0, days: 0, hours: 0, seconds: 0, weeks: 0 }); // 180

I know this library servers another purpose but as it already contains such functionality and it is exposed to the user, why not make it as simple as possible and require as small object as possible to calculate the value.

ZigaStrgar commented 3 years ago

@tolu

tolu commented 3 years ago

Hey, thanks for the contribution! 🙏

I absolutely agree that this should be rectified.

I could imagine something like this, where we cover our backs for both cases of using any of the exports directly:

const defaultDuration = Object.freeze({
  years: 0,
  months: 0,
  weeks: 0,
  days: 0,
  hours: 0,
  minutes: 0,
  seconds: 0
});

export const toSeconds = (duration, startDate) => {
   duration = Object.assign({}, defaultDuration, duration)
   // ...
}
export const end = (duration, startDate) => {
  duration = Object.assign({}, defaultDuration, duration)
  // ...
}

What do you think @ZigaStrgar?

ZigaStrgar commented 3 years ago

@tolu You are welcome and I absolutely agree with your proposition and I've already made a commit to reflect that.