Editorial: Pass Duration Records rather than individual components

[gibson042]

Many operations seem to be simplified by dealing with Duration Records rather than individual fields.

And I suspect that even further reform in this direction remains possible.

[codecov]

Codecov Report

Merging #2290 (9370ef0) into main (1d42297) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2290   +/-   ##
=======================================
  Coverage   91.08%   91.08%           
=======================================
  Files          19       19           
  Lines       10566    10566           
  Branches     1695     1695           
=======================================
  Hits         9624     9624           
  Misses        932      932           
  Partials       10       10           

Flag	Coverage Δ
test262	83.94% <ø> (ø)
tests	81.83% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d42297...9370ef0. Read the comment docs.

[anba]

This change is normative, because it replaces two AddZonedDateTime calls with a single call to AddZonedDateTime. Apart from the obvious normative changes (e.g. fewer observable lookups), will this change result in any other changes, i.e. was there a reason to perform two calls?

[gibson042]

Nice catch! I have reverted the normative change and referenced this comment from #2289.

As for whether or not the separate calls make sense, I don't think so. They seem harmless but unnecessary for Temporal.Duration.prototype.{add,subtract}, and undesirable for Temporal.Duration.prototype.round and Temporal.ZonedDateTime.prototype.{since,until}.

[Ms2ger]

Hmm, this is not as obvious as it seems. CreateDurationRecord does a MV → float → MV dance. Are you sure none of these changes are affected?

[gibson042]

I corrected this to remain in MV domain with a later commit.

[anba]

Nit: Typo CreateTemporalDurationRecord should be CreateDurationRecord throughout this commit.

CreateDurationRecord is fallible, because days can have a different sign than the time components:

let d = Temporal.Duration.from({
  weeks: 1,
  days: 0,
  hours: 1,
});

let cal = new class extends Temporal.Calendar {
  #dateAdd = 0;
  dateAdd(date, duration, options) {
    if (++this.#dateAdd === 1) {
      duration = "-P1W";
    }
    return super.dateAdd(date, duration, options);
  }
}("iso8601");

let zdt = new Temporal.ZonedDateTime(0n, "UTC", cal);
let r = d.total({
  relativeTo: zdt,
  unit: "days",
});
console.log("rounded result=", r);

[gibson042]

I updated this spot in BalanceDuration where the changes were introducing a risk, but am rather confident that the other invocations are safe. I'm less confident that the use of CreateTemporalDuration within AddZonedDateTime is safe, but that is not changed by this PR.

[ptomato]

I'm not in favour of this change because I find it makes the rendered spec text less readable in two ways:

• The sticky highlighting becomes less useful. When auditing the uses of BalanceDuration, for example, I found it a lifesaver to be able to highlight years, months, weeks, days, etc. in different colors. If they become durationFieldsRecord.[[Years]] etc., that's no longer possible.
• Records with as many fields as Duration Records have are rendered in a kind of jumbled-up way. e.g.

1. Let _record_ be the Record {
    [[Years]]: _foo_,
    [[Months]]: _bar_,
    [[Weeks]]: 0,
  }.

is easy to scan visually in the ecmarkup source, but when rendered it becomes

Let record be the Record { [[Years]]: foo, [[Months]]: bar, [[Weeks]]: 0, ... }.

which in my opinion is much harder to visually scan, and makes it hard to notice discrepancies (whether accidental or intentional) between the way the fields are assigned. (for example, the 0 above)

[gibson042]

I find the current spec to be hard to read precisely because there are so many aliases, especially in operation signatures (cf. tc39/ecmarkup#459 for a relevant consequence). But perhaps there's a compromise position in which the operations deal in Records but those Records theirselves are never constructed manually but rather always through a single (or small handful of) operations that have a distinct parameter for each unit?

And it's also conceivable that spec highlighting should be extended to include [[…]] contents.

[ptomato]

Is highlighting of e.g. record.[[Foo]] something that the ecmarkup maintainers would be interested in implementing? I don't think I will have time to do it myself.

[ptomato]

@gibson042 Are you planning to continue this PR?

[justingrant]

Is this PR still moving fwd? Seems like we should either close it or merge it somewhat soon so we can have a clean plate in preparation for Stage 4.

[gibson042]

Yes, and (for my own future reference) @ptomato recommends rebasing on #2519.

[justingrant]

Is this PR going to move fwd?

@ptomato - do you have an opinion about whether we should continue to do wide-ranging editorial changes like this one, or should pause for a while to give implementers a more stable spec to work on?

[ptomato]

I'd prefer we try to get them in quickly after merging the normative changes, and only then stop doing them.