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.

Sign up for GitHub

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

extended jsdoc about duration and units #1564

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

extended jsdoc about duration and units #1564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9366c46
extended jsdoc about duration and units, especially when using Durati…
ptandler Jan 9, 2024
9f0220b
extended jsdoc about duration and units, especially when using Durati…
ptandler Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions src/datetime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2031,17 +2031,35 @@ export default class DateTime {

/**
* Return the difference between two DateTimes as a Duration.
*
* Note that depending on the context you use the duration for, it might be necessary to pass the target units you need,
* as this defaults to milliseconds (see examples below).
*
* When you first calculate the duration as milliseconds and then in a second step shift the units, it's quite likely
Copy link
Member

Choose a reason for hiding this comment

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

While I appreciate that you ran into a specific issue that was confusing, I don' t think we can really document each possible troubleshooting case in here. I think instead we should just say "Math on duration is subtle, please see https://moment.github.io/luxon/#/math".

* that there are cases where you get undesired results, e.g. 1 year -> 31622400000 ms -> 1 year and 6 days (see below)
*
* For details about calculation with duration units, see https://moment.github.io/luxon/#/math
*
* @param {DateTime} otherDateTime - the DateTime to compare this one to
* @param {string|string[]} [unit=['milliseconds']] - the unit or array of units (such as 'hours' or 'days') to include in the duration.
* @param {Object} opts - options that affect the creation of the Duration
* @param {string} [opts.conversionAccuracy='casual'] - the conversion system to use
* @example
* var i1 = DateTime.fromISO('1982-05-25T09:45'),
* i2 = DateTime.fromISO('1983-10-14T10:30');
* i2.diff(i1).toObject() //=> { milliseconds: 43807500000 }
* i2.diff(i1, 'hours').toObject() //=> { hours: 12168.75 }
* i2.diff(i1, ['months', 'days']).toObject() //=> { months: 16, days: 19.03125 }
* i2.diff(i1, ['months', 'days', 'hours']).toObject() //=> { months: 16, days: 19, hours: 0.75 }
* i2.diff(i1).toObject() // => { milliseconds: 43807500000 }
* i2.diff(i1, 'hours').toObject() // => { hours: 12168.75 }
* i2.diff(i1, ['months', 'days']).toObject() // => { months: 16, days: 19.03125 }
* i2.diff(i1, ['months', 'days', 'hours']).toObject() // => { months: 16, days: 19, hours: 0.75 }
*
* var d1 = DateTime.fromISO('2001-01-01T00:00:00').diff(DateTime.fromISO('2000-01-01T00:00:00'))
* // => { milliseconds: 31622400000 }
* var d2 = d1.shiftTo('years', 'months', 'days', 'hours', 'minutes', 'seconds')
* // => { years: 1, months: 0, days: 6, hours: 0, minutes: 0, seconds: 0 }
* var d3 = DateTime.fromISO('2001-01-01T00:00:00').diff(DateTime.fromISO('2000-01-01T00:00:00'),
* ['years', 'months', 'days', 'hours', 'minutes', 'seconds'])
* // => { years: 1, months: 0, days: 0, hours: 0, minutes: 0, seconds: 0 }
* @see Duration#shiftTo
* @return {Duration}
*/
diff(otherDateTime, unit = "milliseconds", opts = {}) {
Expand Down
42 changes: 37 additions & 5 deletions src/duration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,8 @@ export default class Duration {
* @param {Object} opts - options for parsing
* @param {string} [opts.locale='en-US'] - the locale to use
* @param {string} opts.numberingSystem - the numbering system to use
* @param {string} [opts.conversionAccuracy='casual'] - the conversion system to use
* @param {string} [opts.conversionAccuracy='casual'] - the conversion system to use,
* see https://moment.github.io/luxon/#/math
* @return {Duration}
*/
static fromMillis(count, opts) {
Expand All @@ -280,7 +281,8 @@ export default class Duration {
* @param {Object} [opts=[]] - options for creating this Duration
* @param {string} [opts.locale='en-US'] - the locale to use
* @param {string} opts.numberingSystem - the numbering system to use
* @param {string} [opts.conversionAccuracy='casual'] - the preset conversion system to use
* @param {string} [opts.conversionAccuracy='casual'] - the preset conversion system to use,
* see https://moment.github.io/luxon/#/math
* @param {string} [opts.matrix=Object] - the custom conversion system to use
* @return {Duration}
*/
Expand Down Expand Up @@ -331,7 +333,8 @@ export default class Duration {
* @param {Object} opts - options for parsing
* @param {string} [opts.locale='en-US'] - the locale to use
* @param {string} opts.numberingSystem - the numbering system to use
* @param {string} [opts.conversionAccuracy='casual'] - the preset conversion system to use
* @param {string} [opts.conversionAccuracy='casual'] - the preset conversion system to use,
* see https://moment.github.io/luxon/#/math
* @param {string} [opts.matrix=Object] - the preset conversion system to use
* @see https://en.wikipedia.org/wiki/ISO_8601#Durations
* @example Duration.fromISO('P3Y6M1W4DT12H30M5S').toObject() //=> { years: 3, months: 6, weeks: 1, days: 4, hours: 12, minutes: 30, seconds: 5 }
Expand All @@ -354,7 +357,8 @@ export default class Duration {
* @param {Object} opts - options for parsing
* @param {string} [opts.locale='en-US'] - the locale to use
* @param {string} opts.numberingSystem - the numbering system to use
* @param {string} [opts.conversionAccuracy='casual'] - the preset conversion system to use
* @param {string} [opts.conversionAccuracy='casual'] - the preset conversion system to use,
Copy link
Member

Choose a reason for hiding this comment

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

e.g. this is good

* see https://moment.github.io/luxon/#/math
* @param {string} [opts.matrix=Object] - the conversion system to use
* @see https://en.wikipedia.org/wiki/ISO_8601#Times
* @example Duration.fromISOTime('11:22:33.444').toObject() //=> { hours: 11, minutes: 22, seconds: 33, milliseconds: 444 }
Expand Down Expand Up @@ -725,6 +729,9 @@ export default class Duration {

/**
* Return the length of the duration in the specified unit.
*
* Note as this uses {@link Duration#shiftTo} internally, it can cause the unexpected effects as described there.
Copy link
Member

Choose a reason for hiding this comment

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

Instead, this should say something like:

Uses {@link Duration#shiftTo} to convert to the request unit. See the documentation there for details.

The reason this is better is that "unexpected" depends on your expectations. We don't want a warning message; we want a help you understand message.

*
* @param {string} unit - a unit such as 'minutes' or 'days'
* @example Duration.fromObject({years: 1}).as('days') //=> 365
* @example Duration.fromObject({years: 1}).as('months') //=> 12
Expand Down Expand Up @@ -759,6 +766,9 @@ export default class Duration {

/**
* Rescale units to its largest representation
*
* Note as this uses {@link Duration#shiftTo} internally, it can cause the unexpected effects as described there.
*
* @example Duration.fromObject({ milliseconds: 90000 }).rescale().toObject() //=> { minutes: 1, seconds: 30 }
* @return {Duration}
*/
Expand All @@ -770,7 +780,25 @@ export default class Duration {

/**
* Convert this Duration into its representation in a different set of units.
* @example Duration.fromObject({ hours: 1, seconds: 30 }).shiftTo('minutes', 'milliseconds').toObject() //=> { minutes: 60, milliseconds: 30000 }
*
* Note that using shiftTo() can sometimes have unexpected results when using units that don't refer to a
Copy link
Member

Choose a reason for hiding this comment

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

Here too. Now like 95% of the doc for this is a troubleshooting guide for conversionAccuracy. I think here we just say:

Converting between units is subtle because units don't always have constant length. Please see https://moment.github.io/luxon/#/math?id=casual-vs-longterm-conversion-accuracy

* constant duration, such as months or years (see example below). In these cases shiftTo() uses a constant factor
* to calculate with these units as described at https://moment.github.io/luxon/#/math?id=casual-vs-longterm-conversion-accuracy
*
* If you can't avoid using shiftTo() in these cases, you can choose which conversion scheme is appropriate in
* your case which you can pass when constructing a Duration with the `conversionAccuracy` option,
* see e.g. {@link Duration.fromObject}.
* If you construct a Duration using {@link Duration#diff}, you can pass the desired target units directly.
*
* @example Duration.fromObject({ hours: 1, seconds: 30 }).shiftTo('minutes', 'milliseconds').toObject() // => { minutes: 60, milliseconds: 30000 }
*
* @example
* var d1 = DateTime.fromISO('2001-01-01T00:00:00').diff(DateTime.fromISO('2000-01-01T00:00:00'))
* // => { milliseconds: 31622400000 }
* var d2 = d1.shiftTo('years', 'months', 'days', 'hours', 'minutes', 'seconds')
* // => { years: 1, months: 0, days: 6, hours: 0, minutes: 0, seconds: 0 }
* // Oops, we got 6 days more than we had before!
*
* @return {Duration}
*/
shiftTo(...units) {
Expand Down Expand Up @@ -832,6 +860,10 @@ export default class Duration {
/**
* Shift this Duration to all available units.
* Same as shiftTo("years", "months", "weeks", "days", "hours", "minutes", "seconds", "milliseconds")
*
* Note as this uses {@link Duration#shiftTo} internally, it can cause the unexpected effects as described there.
*
* @see Duration#shiftTo
* @return {Duration}
*/
shiftToAll() {
Expand Down