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

extended jsdoc about duration and units #1564

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

Conversation

ptandler
Copy link
Contributor

@ptandler ptandler commented Jan 9, 2024

especially when using Duration#diff or Duration#shiftTo

Copy link

linux-foundation-easycla bot commented Jan 9, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@ptandler
Copy link
Contributor Author

@icambron After understanding a bug I had using Luxon, I thought it might be a good idea to add a bit to the documentation so others might prevent running into the same problem.

Copy link
Member

@icambron icambron left a comment

Choose a reason for hiding this comment

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

I really appreciate the effort you put into this. I've asked for some changes, mostly because the doc strings can't really be troubleshooting guides, but can be links out to detailed explanations in the other docs.

* 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".

@@ -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

@@ -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.

@@ -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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants