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

Can the overlap between Duration instances and Duration Records be eliminated? #2308

Closed
gibson042 opened this issue Jun 16, 2022 · 2 comments · Fixed by #2943
Closed

Can the overlap between Duration instances and Duration Records be eliminated? #2308

gibson042 opened this issue Jun 16, 2022 · 2 comments · Fixed by #2943
Assignees
Labels
editorial spec-text Specification text involved
Milestone

Comments

@gibson042
Copy link
Collaborator

Copied from #2281 (review)

Both Duration instances and Duration Records have the same set of immutable fields (with an extra branding field for the former), and the spec provides no internal guidance about when to use one vs. the other, resulting in muddled treatment1.

I think Duration Record should be eliminated altogether, or else Duration instances should be refactored to have a field containing a Duration Record rather than duplicating its fields.

Footnotes

  1. Temporal.Duration.compare and Temporal.Calendar.prototype.dateAdd include surprising examples, and the apparent pattern of DifferenceTemporal… operations returning Temporal.Duration instances vs. other Difference… operations returning Records or in some cases mathematical values is an extremely subtle one and appears to be wholly undocumented.

@ljharb
Copy link
Member

ljharb commented Jun 16, 2022

I love either of those outcomes.

@ptomato
Copy link
Collaborator

ptomato commented Jun 20, 2022

I believe we had this distinction so that nobody would be tempted to observably call methods on Duration instances in internal algorithms, similar to what we did with the other types. I do like that separation because it is clear that if you have a Duration Record, it should never be user-visible.

Also because for a long time it wasn't clear, at least to me, under what circumstances OrdinaryCreateFromConstructor and therefore CreateTemporalDuration could throw.

The use of Duration instances internally in the newly-added DifferenceTemporal... operations is something I personally don't like, but the amount of deduplication enabled by those operations outweighed the disadvantage for me.

@ptomato ptomato added editorial spec-text Specification text involved labels Dec 8, 2022
@ptomato ptomato added this to the Stage 4 milestone Dec 8, 2022
@ptomato ptomato self-assigned this Sep 13, 2024
@ptomato ptomato modified the milestones: Stage 4, Stage "3.5" Sep 19, 2024
ptomato added a commit that referenced this issue Sep 20, 2024
Also introduce operations to take the sign of a Date Duration Record and
of a Normalized Duration Record, which replace some uses of DurationSign
where there wasn't a Duration instance handy to pass to it.

See: #2308
ptomato added a commit that referenced this issue Sep 20, 2024
ptomato added a commit that referenced this issue Sep 20, 2024
Instead of picking a _sign_ of -1 or 1 and multiplying the components by
it, create the Duration instance and then pass it through
CreateNegatedTemporalDuration if the operation is ~since~.

See: #2308
ptomato added a commit that referenced this issue Sep 20, 2024
In the reference code, move AddISODate into calendar.mjs because it's only
used once there (and replace the other calls of it with BalanceISODate.)
This is a small step towards untangling the circular import in
calendar.mjs.

See: #2308
ptomato added a commit that referenced this issue Sep 20, 2024
Similar to DifferenceTemporal___, instead of picking a _sign_ of -1 or 1
and multiplying the duration components by it, create the Duration
instance and then pass it through CreateNegatedTemporalDuration if the
operation is ~subtract~.

This was already started in #2290, and brought into consistency everywhere
in this commit.

See: #2308
ptomato added a commit that referenced this issue Sep 20, 2024
…normalize

This is a pattern that we already mostly use, but now that everything is
refactored to take Records instead of individual Duration components, it
can be expressed much more concisely.

See: #2308
ptomato added a commit that referenced this issue Sep 20, 2024
ptomato added a commit that referenced this issue Sep 20, 2024
…c-unnormalize

This allows inlining AddDateTime into AddDurationTo...PlainDateTime. It's
the only place AddDateTime was used.

See: #2308
ptomato added a commit that referenced this issue Sep 20, 2024
ptomato added a commit that referenced this issue Sep 20, 2024
In a few places, we did the operation of balancing the normalized time
duration up to days, adding the days to the [[Days]] field of the date
duration record, and discarding the time duration. Encapsulate this in its
own AO.

See: #2308
ptomato added a commit that referenced this issue Sep 20, 2024
BalanceTimeDuration is now only used in UnnormalizeDuration. Inline it
there.

There is no longer any usage of Time Duration Records, so remove them.

See: #2308
Ms2ger pushed a commit that referenced this issue Sep 20, 2024
Ms2ger pushed a commit that referenced this issue Sep 20, 2024
In a few places, we did the operation of balancing the normalized time
duration up to days, adding the days to the [[Days]] field of the date
duration record, and discarding the time duration. Encapsulate this in its
own AO.

See: #2308
Ms2ger pushed a commit that referenced this issue Sep 20, 2024
BalanceTimeDuration is now only used in UnnormalizeDuration. Inline it
there.

There is no longer any usage of Time Duration Records, so remove them.

See: #2308
Ms2ger pushed a commit that referenced this issue Sep 20, 2024
In a few places, we want to create a new Date Duration Record but with one
or more fields adjusted, as if Date Duration Records had a with() method.
This is easy to do with spreading and destructuring in JS, but is unwieldy
in spec language. Add an AdjustDateDurationRecord abstract operation to do
this more concisely.

See: #2308
Ms2ger pushed a commit that referenced this issue Sep 20, 2024
Also introduce operations to take the sign of a Date Duration Record and
of a Normalized Duration Record, which replace some uses of DurationSign
where there wasn't a Duration instance handy to pass to it.

See: #2308
Ms2ger pushed a commit that referenced this issue Sep 20, 2024
Ms2ger pushed a commit that referenced this issue Sep 20, 2024
Instead of picking a _sign_ of -1 or 1 and multiplying the components by
it, create the Duration instance and then pass it through
CreateNegatedTemporalDuration if the operation is ~since~.

See: #2308
Ms2ger pushed a commit that referenced this issue Sep 20, 2024
In the reference code, move AddISODate into calendar.mjs because it's only
used once there (and replace the other calls of it with BalanceISODate.)
This is a small step towards untangling the circular import in
calendar.mjs.

See: #2308
Ms2ger pushed a commit that referenced this issue Sep 20, 2024
Similar to DifferenceTemporal___, instead of picking a _sign_ of -1 or 1
and multiplying the duration components by it, create the Duration
instance and then pass it through CreateNegatedTemporalDuration if the
operation is ~subtract~.

This was already started in #2290, and brought into consistency everywhere
in this commit.

See: #2308
Ms2ger pushed a commit that referenced this issue Sep 20, 2024
…normalize

This is a pattern that we already mostly use, but now that everything is
refactored to take Records instead of individual Duration components, it
can be expressed much more concisely.

See: #2308
Ms2ger pushed a commit that referenced this issue Sep 20, 2024
Ms2ger pushed a commit that referenced this issue Sep 20, 2024
…c-unnormalize

This allows inlining AddDateTime into AddDurationTo...PlainDateTime. It's
the only place AddDateTime was used.

See: #2308
Ms2ger pushed a commit that referenced this issue Sep 20, 2024
Ms2ger pushed a commit that referenced this issue Sep 20, 2024
In a few places, we did the operation of balancing the normalized time
duration up to days, adding the days to the [[Days]] field of the date
duration record, and discarding the time duration. Encapsulate this in its
own AO.

See: #2308
Ms2ger pushed a commit that referenced this issue Sep 20, 2024
BalanceTimeDuration is now only used in UnnormalizeDuration. Inline it
there.

There is no longer any usage of Time Duration Records, so remove them.

See: #2308
Ms2ger pushed a commit that referenced this issue Sep 20, 2024
In a few places, we want to create a new Date Duration Record but with one
or more fields adjusted, as if Date Duration Records had a with() method.
This is easy to do with spreading and destructuring in JS, but is unwieldy
in spec language. Add an AdjustDateDurationRecord abstract operation to do
this more concisely.

See: #2308
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial spec-text Specification text involved
Projects
None yet
3 participants