-
Notifications
You must be signed in to change notification settings - Fork 153
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
Editorial: Pass Duration Records rather than individual components #2290
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
6bcbe92
to
da0a7e6
Compare
da0a7e6
to
ec0f2a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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. |
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. |
@gibson042 Are you planning to continue this PR? |
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. |
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? |
I'd prefer we try to get them in quickly after merging the normative changes, and only then stop doing them. |
Now that we have sticky highlighting of record fields, I'm much more positive about this. I've rebased the PR. Unfortunately a lot of it refactored AOs that no longer exist (because they were substantially rewritten in #2758.) I've kept what still applies, dropped the rest, and written the corresponding code for the reference polyfill. I think we should merge this now before it gets stale again. I see where it is going, however, and have a bunch of ideas for continued simplifications of this type, which I will pursue as part of #2308. |
9370ef0
to
062f2da
Compare
Implements the previous commit in the reference code.
…dividiual aliases
Implements the previous commit in the reference code.
…ther than individual fields
Implements the previous commit in the reference code.
… text Implements the previous commit in the reference code.
062f2da
to
0a24165
Compare
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
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
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
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.