-
Notifications
You must be signed in to change notification settings - Fork 105
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: Normalize internal slot access #603
Conversation
@@ -135,9 +135,9 @@ <h1>Internal Slots</h1> | |||
</p> | |||
|
|||
<ul> | |||
<li>The first element of [[SortLocaleData]][[<_locale_>]].[[co]] and [[SearchLocaleData]][[<_locale_>]].[[co]] must be *null*.</li> | |||
<li>The first element of [[SortLocaleData]].[[<_locale_>]].[[co]] and [[SearchLocaleData]].[[<_locale_>]].[[co]] must be *null*.</li> |
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.
this was really confusing to read at first (either form) because it's implying that [[SortLocaleData]]
is a first-class thing. Perhaps it would be clearer if it said something like this:
<li>The first element of [[SortLocaleData]].[[<_locale_>]].[[co]] and [[SearchLocaleData]].[[<_locale_>]].[[co]] must be *null*.</li> | |
<li>The first element in any object `_obj_` of _obj_.[[SortLocaleData]].[[<_locale_>]].[[co]] and [[SearchLocaleData]].[[<_locale_>]].[[co]] must be *null*.</li> |
?
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.
That would need to be %Collator%.[[SortLocaleData]], not obj.[[SortLocaleData]]. (Especially because that uses obj in the definition of what obj refers to)
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.
sure, if there's a specific object here then that's the obvious choice :-) it's just very unclear to me to have an internal slot used like this.
Changes `<_slot_>` to `<_slot_>` so it marks up properly.
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.
LGTM. I think @ljharb's comments are important but not blocking since they apply with or without this PR (and to place outside of this diff as well).
Jordan do you agree? Can I merge this as-is?
Yes, since it’s confusing in either form, it’s fine to merge this as-is; but it’d be great to clean it up ASAP :-) |
@ljharb agreed. Would you like to file an issue for it or should I? I plan to address a bunch of editorial issues soon so that would put it on my personal TODO list. |
Filed as #612. |
This PR changes several instances of "[[Slot]][[<variable>]]" and "[[Slot]][<variable>]" to the proper "[[Slot]].[[<variable>]]" notation.
Thanks to @hackmud-dtr for noticing these.