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

Add space to class string of iterator objects #483

Merged
merged 3 commits into from
Dec 7, 2017

Conversation

TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Dec 3, 2017

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

@tobie
Copy link
Collaborator

tobie commented Dec 5, 2017

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 ("<code>foo</code>")?

@annevk
Copy link
Member

annevk commented Dec 5, 2017

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.

@tobie
Copy link
Collaborator

tobie commented Dec 5, 2017

So would you say:

Note: In the ECMAScript language binding, an interface that is iterable
will have "<code>entries</code>", "<code>forEach</code>", "<code>keys</code>",
"<code>values</code>" and {{@@iterator}} properties on its [=interface prototype object=].

…or would you use another notation there? And if so, which one?

@annevk
Copy link
Member

annevk commented Dec 5, 2017

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 foo() method or bar attribute so it doesn't come up as much.

@tobie
Copy link
Collaborator

tobie commented Dec 5, 2017

So ECMAScript generally uses:

the <code>foo</code> property

… though it sometimes (albeit rarely) adds quotes:

the <code>"foo"</code> property

@domenic
Copy link
Member

domenic commented Dec 5, 2017

In general I like referring to properties, methods, and class names without quotes, using <code> instead. That's also what's encouraged by Bikeshed's default conventions, e.g. when you do {{EventTarget}}, there are no quotes generated in the output. That speaks to examples like

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 <code>interface</code>, or

{{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?

@TimothyGu
Copy link
Member Author

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 property named " versus 1 instance of property named; 7 instances of "</code> property versus more than 150 instances of [^"]</code> property or [^"]</code> data property). This also works well with the variable/value equivalence: "if A has a method named P" works better than "if A has a P method".

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>",
Copy link
Collaborator

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>"?

Copy link
Member Author

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.

Copy link
Collaborator

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|
Copy link
Collaborator

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|
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

@tobie tobie merged commit 4fcfaea into whatwg:master Dec 7, 2017
@TimothyGu TimothyGu deleted the iterator branch December 7, 2017 12:23
@TimothyGu
Copy link
Member Author

@bzbarsky
Copy link
Collaborator

@TimothyGu Are there web platform tests for this change, by any chance?

@TimothyGu
Copy link
Member Author

@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.

@bzbarsky
Copy link
Collaborator

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...

TimothyGu added a commit to TimothyGu/webidl that referenced this pull request Dec 24, 2017
Per consensus reached in whatwg#483, class strings should only exist on
iterator prototype objects.
@TimothyGu
Copy link
Member Author

@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.

TimothyGu added a commit to TimothyGu/webidl2js that referenced this pull request Dec 25, 2017
TimothyGu added a commit to TimothyGu/webidl2js that referenced this pull request Dec 25, 2017
TimothyGu added a commit to jsdom/webidl2js that referenced this pull request Dec 25, 2017
tobie pushed a commit that referenced this pull request Feb 23, 2018
Per consensus reached in #483, class strings should only exist on
iterator prototype objects.
Comment on lines +4041 to 4042
<!-- String(it); // Evaluates to "[object SessionManager Iterator]"
// TODO: https://github.com/heycam/webidl/issues/419 -->
Copy link
Contributor

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:

Suggested change
<!-- 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.

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

Successfully merging this pull request may close these issues.

Class string of default iterator object seems to have been accidentally changed?
6 participants