-
Notifications
You must be signed in to change notification settings - Fork 165
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
Add space to class string of iterator objects #483
Conversation
I'd like to hear @annevk and @domenic's thoughts on whether all of the strings modified here fall under the infra string definition. While it's unquestionably the case for some of them, I'm unsure about others. For example, when you talk about the "foo interface property", should "foo" be marked-up as a string ( |
If you're specifically talking about the name exposed somewhere, I'd think so. That's what I did in whatwg/html#3242 anyway. If it's an informal reference of sorts I wouldn't do it. |
So would you say:
…or would you use another notation there? And if so, which one? |
I'm not sure. I guess I'd look what ECMAScript does. I would add an Oxford comma. For platform objects we always talk about the |
So ECMAScript generally uses:
… though it sometimes (albeit rarely) adds quotes:
|
In general I like referring to properties, methods, and class names without quotes, using Note: A leading <span class="char">"_"</span> is used to escape an identifier from looking
-like a reserved word so that, for example, an interface named “interface” can be
-like a reserved word so that, for example, an interface named "<code>interface</code>" can be
defined. The leading <span class="char">"_"</span> is dropped to unescape the
identifier. which should probably be just {{ECMAScript/Set}} objects. If the <emu-t>readonly</emu-t>
-keyword is used, this includes “entries”, “forEach”, “has”,
-“keys”, “values”, {{@@iterator}} methods and a “size” getter.
-For read–write setlikes, it also includes “add”, “clear”, and “delete” methods.
+keyword is used, this includes "<code>entries</code>", "<code>forEach</code>", "<code>has</code>",
+"<code>keys</code>", "<code>values</code>", {{@@iterator}} methods and a "<code>size</code>" getter.
+For read–write setlikes, it also includes "<code>add</code>", "<code>clear</code>", and "<code>delete</code>" methods. where again the quotes are probably not ideal. It's less clear what to do when talking about an identifier, or a "name" e.g. as in -Although the “toJSON” [=identifier=] is not a [=reserved identifier=],
+Although the "<code>toJSON</code>" [=identifier=] is not a [=reserved identifier=], or Interfaces that [=support indexed properties=] must define
-an [=integer types|integer-typed=] [=attribute=] named “length”.
+an [=integer types|integer-typed=] [=attribute=] named "<code>length</code>". Probably identifiers and names are strings? I imagine the line gets blurry sometimes. E.g. if we modified the methods listing above to say "methods named ..." instead of "... methods", we'd need to add quotes everywhere? |
I'm comfortable with drawing the line here. This is also what ECMAScript spec does (7 instances of |
index.bs
Outdated
actual [=array iterator objects=]. | ||
|
||
Interfaces with iterable declarations must not | ||
have any [=interface members=] | ||
named “entries”, “forEach”, “keys” or “values”, | ||
named "<code class="idl">entries</code>", "<code class="idl">forEach</code>", |
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.
Is this "<code class="idl">entries</code>"
on purpose or should it be: "<code>entries</code>"
?
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.
It’s that way on purpose. I decided to use idl class for identifiers that are not plain strings.
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.
Mmm. You're consciously making the decision to mark those up differently than idl props (which aren't quoted), but you don't want to mark those as "infra" strings. Why not just "entries"
, then?
index.bs
Outdated
|
||
When the internal \[[DefineOwnProperty]] method of a [=legacy platform object=] |O| | ||
When the \[[DefineOwnProperty]] method of a [=legacy platform object=] |O| |
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.
Missing an "internal" here
index.bs
Outdated
|
||
The internal \[[Delete]] method of every [=legacy platform object=] |O| | ||
The \[[Delete]] method of every [=legacy platform object=] |O| |
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.
Same here.
@TimothyGu Are there web platform tests for this change, by any chance? |
@bzbarsky I don't think WPT has any tests for toStringTag yet (for anything, iterator objects and regular objects), because of the fact that different browser vendors haven't implemented it. So, no. |
OK, but we seem to sort of have browser consensus on the behavior specifically for iterator objects, right? And the spec reflects that consensus? If so, seems like we can add tests; tests don't need to wait on implementation... |
Per consensus reached in whatwg#483, class strings should only exist on iterator prototype objects.
@bzbarsky I've opened #501 to make the spec fully consistent with browser consensus. Tests for both this PR and #501 are in web-platform-tests/wpt#8796. |
Per consensus reached in #483, class strings should only exist on iterator prototype objects.
<!-- String(it); // Evaluates to "[object SessionManager Iterator]" | ||
// TODO: https://github.com/heycam/webidl/issues/419 --> |
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 should’ve been uncommented in this PR:
<!-- String(it); // Evaluates to "[object SessionManager Iterator]" | |
// TODO: https://github.com/heycam/webidl/issues/419 --> | |
String(it); // Evaluates to "[object SessionManager Iterator]" |
I’m fixing this in #858.
The first commit changes the notation of strings within Web IDL to Infra-mandated "
string
". This helps prevent errors like #419 to exist in the first place, as it can be difficult to tell the difference between " Iterator" and "Iterator" without monospace styling.The second commit fixes a Bikeshed linking error.
The third commit is a normative change that actually fixes #419, by incorporating the consensus reached in #419 (comment).
Preview | Diff