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

Editorial: Reorder table fields by magnitude #2429

Merged
merged 7 commits into from
Oct 26, 2022

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Oct 25, 2022

This implements the part of #2254 that is possible to do without any changes that are observable from JS. It reorders the tables of Duration Record fields and TemporalTimeLike Record fields so that the fields are in magnitude order, and enforces the property access in alphabetical order by sorting the list of fields. The net result is that nothing changes, but the spec text is more robust against accidental observable modifications.

Also includes some minor editorial and docs fixes.

@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

Merging #2429 (50fef67) into main (96e0e3e) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 50fef67 differs from pull request most recent head e05f8f2. Consider uploading reports for the commit e05f8f2 to get more accurate results

@@            Coverage Diff             @@
##             main    #2429      +/-   ##
==========================================
+ Coverage   94.65%   94.66%   +0.01%     
==========================================
  Files          20       20              
  Lines       11070    11074       +4     
  Branches     1970     1970              
==========================================
+ Hits        10478    10483       +5     
  Misses        544      544              
+ Partials       48       47       -1     
Impacted Files Coverage Δ
polyfill/lib/ecmascript.mjs 98.36% <0.00%> (+0.02%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

spec/duration.html Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
spec/mainadditions.html Outdated Show resolved Hide resolved
spec/mainadditions.html Outdated Show resolved Hide resolved
@ptomato
Copy link
Collaborator Author

ptomato commented Oct 25, 2022

I realized that we actually don't have to alphabetize the fields in ToTemporalTimeRecord. The access order is unobservable, because we are accessing the properties on the Object returned from PrepareTemporalFields, which is guaranteed to be a null-prototype ordinary Object with only data properties.

Based on the discussion in the TC39 Matrix channel, it also seems that at least two ECMA-262 editors have a strong preference that we do not loop over Record fields, but instead unroll the loop.

@ptomato ptomato force-pushed the unobservable-table-access-order branch from d67bdd9 to 50fef67 Compare October 25, 2022 19:12
@ptomato
Copy link
Collaborator Author

ptomato commented Oct 25, 2022

I've added another commit that writes out all the field accesses explicitly, based on the Matrix discussion.

This is usually the case for dates in the modern era, but it's not true
for dates that use extended years. Therefore, it's misleading to recommend
it.
Since this language now occurs several times in the spec, we create an
abstract operation for it, SortStringListByCodeUnit. (The default sort
order of Array.prototype.sort sorts strings as sequences of UTF-16 code
units.)
Keeping the table in alphabetical order risks later observable reordering
in an edit by someone who doesn't realize the order is significant. It's
also less confusing to have the table in an intuitive order, and more
explicit about what we are doing to sort the property names before
accessing them.

(Based on Richard's suggestion in the issue description.)

See: #2254
Unlike Duration, the table order is not significant here. The Object
returned from PrepareTemporalFields is guaranteed to be a null-prototype
ordinary Object with only data properties. So we may as well put the table
in an intuitive order.

See: #2254
Based on a discussion in the TC39 Matrix, it seems that at least two of
the ECMA-262 editors have a strong preference for this.
@ptomato ptomato force-pushed the unobservable-table-access-order branch from 50fef67 to e05f8f2 Compare October 26, 2022 18:35
@ptomato ptomato merged commit 8e80575 into main Oct 26, 2022
@ptomato ptomato deleted the unobservable-table-access-order branch October 26, 2022 18:38
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