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

Rebase some more upstream changes #266

Merged
merged 4 commits into from
Jan 12, 2024
Merged

Conversation

12wrigja
Copy link
Contributor

No description provided.

@12wrigja 12wrigja marked this pull request as ready for review October 13, 2023 17:53
@12wrigja 12wrigja requested a review from justingrant October 13, 2023 17:54
gibson042 and others added 4 commits January 2, 2024 08:00
Other call sites always provide an Object, so this isolates the inconsistency.

UPSTREAM_COMMIT=f95cde5996287897ee290f4a9690742fc944d659
BalanceDuration → BalanceTimeDuration
BalancePossiblyInfiniteDuration → BalancePossiblyInfiniteTimeDuration
BalanceDurationRelative → BalanceDateDurationRelative
UnbalanceDurationRelative → UnbalanceDateDurationRelative

This makes it clearer what portion of the duration units the operation
operates on; especially since we will represent time durations in
normalized form.

UPSTREAM_COMMIT=cbec236534791352ca412efd481f858e4e5b436a
BalanceTimeDuration (formerly BalanceDuration) had two code paths tangled
up inside of it: one where days always equal 24 hours (duration is not
relative to a ZonedDateTime) and one where days may be different from 24
hours (duration is relative to a ZonedDateTime). Splitting these two code
paths out into two different AOs makes them both easier to understand and
reason about.

It also removes the need to manually indicate that BalanceTimeDuration
doesn't call into user code, because only BalanceTimeDurationRelative can
call into user code.

UPSTREAM_COMMIT=0547d29dbdf4c5aa9c2dca3c6072513dc52abe36
Similar to the previous commit, this makes NanosecondsToDays easier to
understand and reason about. After splitting BalanceTimeDuration and
BalanceTimeDurationRelative, there was only one remaining call site of
NanosecondsToDays with a possible non-ZonedDateTime relativeTo, in
RoundDuration. Instead, inline the 24-hour day case into the call site.

UPSTREAM_COMMIT=dffb9298e0c95c8212a624058652ee7f8dba2cdd
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

These all look correct. 👍

@12wrigja 12wrigja merged commit b510572 into js-temporal:main Jan 12, 2024
19 checks passed
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.

3 participants