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: Arrange contents consistently #923

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ben-allen
Copy link
Contributor

Resolves the following aspects of #405:

Placed Prototype Object properties in the following order for all Intl Objects:

  1. Intl.Foo.prototype.constructor
  2. Intl.Foo.prototype.resolvedOptions ()
  3. Intl.Foo.prototype [ %Symbol.toStringTag% ]
  4. ...Foo-specific properties in alphabetic order...

Gathered all Abstract Operations associated with each Object into the Abstract Operations section for that Object. Previously Intl.DateTimeFormat, Intl.Locale, and Intl.NumberFormat had several Abstract Operations appear inside the constructor clauses of those Objects.

@anba
Copy link
Contributor

anba commented Sep 12, 2024

ECMA-262 sorts built-in properties alphabetically, with Symbol-keyed properties being sorted to the end. Does it make sense to follow in ECMA-402?

@gibson042
Copy link
Contributor

I'm ambivalent about that, especially dropping resolvedOptions at a varying position for each prototype—there's something to be said for a simple rule, but also for tweaking that rule to highlight internal consistency. On balance, I think I prefer the latter, ideally including documentation justifying it.

@ben-allen ben-allen changed the title Arrange contents consistently Editorial: Arrange contents consistently Sep 12, 2024
@ben-allen
Copy link
Contributor Author

ben-allen commented Oct 11, 2024

My preference strongly matches @gibson042's -- resolvedOptions in predictable place, add text to the style guide explaining the choice. Should we discuss it at TG2? @anba

@ben-allen ben-allen added the editorial Involves an editorial fix label Oct 11, 2024
Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Looking at the rendered spec, I think I do have a slight preference for Symbol.toStringTag properties being after string-keyed properties—especially since they might not even be universal in the future.

Comment on lines 23 to 30
1. Return ? ChainDateTimeFormat(_dateTimeFormat_, NewTarget, _this_).
1. Return _dateTimeFormat_.
</emu-alg>

<emu-normative-optional>
<emu-clause id="sec-chaindatetimeformat" type="abstract operation">
<h1>
ChainDateTimeFormat (
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking, but I have a moderate preference for constructor-specific operations being in their respective Constructor sections, e.g.

  • DateTimeFormat Objects
    1. The Intl.DateTimeFormat Constructor
      1. Intl.DateTimeFormat ( [ locales [ , options ] ] )
      2. CreateDateTimeFormat ( newTarget, locales, options, required, defaults )
      3. ChainDateTimeFormat ( dateTimeFormat, newTarget, this )
      4. DateTimeStyleFormat ( dateStyle, timeStyle, styles )

In addition to locality benefits, it reduces the divergence between constructors that use such helper operations and those that do not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this one! I was torn between leaving them where they were or putting them down with the rest of the AOs. The least-bad argument I have for moving them down is the possibility of those AOs getting called from elsewhere in the spec -- though upon brief perusal the only place that seems to happen is the call to SetNumberFormatDigitOptions from the PluralRules constructor. I'll move them back.

@@ -203,38 +125,6 @@ <h1>Intl.Locale.prototype [ %Symbol.toStringTag% ]</h1>
</p>
</emu-clause>

<emu-clause id="sec-Intl.Locale.prototype.maximize">
Copy link
Contributor

Choose a reason for hiding this comment

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

Intl.Locale is not a service constructor, so we should probably just follow ECMA-262 conventions—string-keyed properties alphabetically, then symbol-keyed properties alphabetically (i.e., Symbol.toStringTag last).

Copy link
Contributor

Choose a reason for hiding this comment

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

Intl.Locale is not a service constructor, so we should probably just follow ECMA-262 conventions—string-keyed properties alphabetically, then symbol-keyed properties alphabetically (i.e., Symbol.toStringTag last).

This one still stands; please move at least Intl.Locale.prototype [ %Symbol.toStringTag% ] to the end of "Properties of the Intl.Locale Prototype Object".

Comment on lines 1216 to 1219
1. If _formatMatcher_ is *"basic"*, then
1. Let _bestFormat_ be BasicFormatMatcher(_formatOptions_, _formats_).
1. Else,
1. Let _bestFormat_ be BestFitFormatMatcher(_formatOptions_, _formats_).
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit for future reference: we should really rename these to DTF-specific operations to indicate that detail (e.g., DateTimeFormat{Basic,BestFit}Matcher).

1. Return the string-concatenation of _sign_, ToZeroPaddedDecimalString(_hours_, 2), the code unit 0x003A (COLON), and ToZeroPaddedDecimalString(_minutes_, 2).
</emu-alg>
</emu-clause>

<emu-clause id="sec-date-time-style-format" type="abstract operation">
<h1>
DateTimeStyleFormat (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit for a followup (if only to minimize review burden): we should also consider alphabetizing these AO lists, and moving/reorganizing where that would hinder comprehension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm provisionally skeptical about alphabetizing the AO lists, largely because the decision on when to break from alphabetical order for higher comprehension is difficult / might introduce inconsistencies that are themselves a source of confusion. I'd almost rather just have the rule be alphabetical order, full stop, no exceptions. Open to arguments otherwise, though -- this isn't a strong preference, and it's likely a somewhat uninformed one.

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, that's in line with my suggestion... the kind of moving/reorganization that I suggest, where necessary, relates to document structure (e.g., FilterLocales and ResolveLocale could be alphabetically sorted as the only children of a "Locale Matching" subsection).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, that makes a lot of sense! I've put it on my todo list

@@ -202,6 +161,47 @@ <h1>Intl.ListFormat.prototype.resolvedOptions ( )</h1>
</table>
</emu-table>
</emu-clause>

<emu-clause id="sec-Intl.ListFormat.prototype-toStringTag">
Copy link
Contributor

@gibson042 gibson042 Oct 12, 2024

Choose a reason for hiding this comment

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

Noticed in passing:

Suggested change
<emu-clause id="sec-Intl.ListFormat.prototype-toStringTag">
<emu-clause id="sec-intl.listformat.prototype-%symbol.tostringtag%" oldids="sec-Intl.ListFormat.prototype-toStringTag">

(and likewise for RelativeTimeFormat—or we could save both for a bigger id cleanup that also covers non-symbol sections)

<emu-normative-optional>
<emu-clause id="sec-chainnumberformat" type="abstract operation">
<h1>
ChainNumberFormat (
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here regarding constructor-specific operations as for DTF.

@ben-allen ben-allen force-pushed the arrange-contents-consistently branch from 3642963 to 2141408 Compare November 1, 2024 19:38
@ben-allen
Copy link
Contributor Author

pushed a commit addressing @gibson042 concerns, largely through dropping the commit that moved constructor-specific AOs down to the section with the rest of the AOs.

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

@@ -203,38 +125,6 @@ <h1>Intl.Locale.prototype [ %Symbol.toStringTag% ]</h1>
</p>
</emu-clause>

<emu-clause id="sec-Intl.Locale.prototype.maximize">
Copy link
Contributor

Choose a reason for hiding this comment

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

Intl.Locale is not a service constructor, so we should probably just follow ECMA-262 conventions—string-keyed properties alphabetically, then symbol-keyed properties alphabetically (i.e., Symbol.toStringTag last).

This one still stands; please move at least Intl.Locale.prototype [ %Symbol.toStringTag% ] to the end of "Properties of the Intl.Locale Prototype Object".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial Involves an editorial fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants