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

Why is options.largestUnit==='days' in Time.prototype.difference? #580

Closed
justingrant opened this issue May 17, 2020 · 3 comments · Fixed by #593
Closed

Why is options.largestUnit==='days' in Time.prototype.difference? #580

justingrant opened this issue May 17, 2020 · 3 comments · Fixed by #593

Comments

@justingrant
Copy link
Collaborator

justingrant commented May 17, 2020

I found the docs for for Time.prototype.difference to be confusing because:

  • 'years', 'months', and 'days' are all accepted even though these fields aren't part of a Time
  • The default value is 'days' which is also not part of a Time.

What I expected:

  • default should be hours
  • years, months, and days should throw OR the docs should clarify that Y/M/D are treated as hours.

I assume that hours will behave identically to days (per #324), so this might just be a documentation issue with no spec or implementation change needed.

Here's the current docs:

largestUnit (string): The largest unit of time to allow in the resulting Temporal.Duration object. Valid values are 'years', 'months', 'days', 'hours', 'minutes', and 'seconds'. The default is days.

@ptomato
Copy link
Collaborator

ptomato commented May 18, 2020

Here's the thread with context for this particular decision, starting from this point: #307 (comment)

days was chosen for consistency, because it's the default for all the other types (except Absolute which is seconds, which I would actually consider reversing, now that we've decided to support calendars with 24-hour solar days)

As for throwing or not throwing, I think the rule of thumb we used was to throw if the value would be harmful or nonsensical (e.g. hours for YearMonth.difference) but not throw if the value was harmless (months for Time.difference`).

@justingrant
Copy link
Collaborator Author

Makes sense, thanks for the context. BTW, I just read lower-down in the docs and they are contradictory:

largestUnit (string): The largest unit of time to allow in the resulting Temporal.Duration object. Valid values are 'years', 'months', 'days', 'hours', 'minutes', and 'seconds'. The default is days.

vs.

The default largest unit in the result is hours. Since this method never returns any duration longer than 12 hours, largest units of years, months, or days, are by definition ignored and treated as hours.

I assume these should be rationalized? Seems simplest just to lie to the user and claim in both places that the default is 'hours'. Users won't be able to tell, right?

For Absolute, I think the ideal answer would be implementing #584 and then setting the default to nanoseconds! ;-) What's powerful about the #584 solution is that you don't have to mix the domains of "how many total minutes are in this Duration?" vs. "what would a stopwatch show for numbers of minutes & seconds for this duration?"

@ptomato
Copy link
Collaborator

ptomato commented May 19, 2020

I'll fix this up.

I assume these should be rationalized? Seems simplest just to lie to the user and claim in both places that the default is 'hours'. Users won't be able to tell, right?

Right, I don't believe they could unless by inspection of the function's source with Temporal.Time.prototype.difference.toString()

ptomato added a commit that referenced this issue May 19, 2020
Clear up this language a bit and explain that the default is effectively
hours.

Closes: #580
ryzokuken pushed a commit that referenced this issue May 20, 2020
Clear up this language a bit and explain that the default is effectively
hours.

Closes: #580
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 a pull request may close this issue.

2 participants