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

spec: Make ShareData argument optional. #51

Merged
merged 1 commit into from
Jul 4, 2017

Conversation

mgiuca
Copy link
Collaborator

@mgiuca mgiuca commented Jul 4, 2017

This has no behavioural change, as passing an empty dictionary is a
TypeError. But it is required by the WebIDL spec.

Closes #47.


Preview | Diff

@mgiuca
Copy link
Collaborator Author

mgiuca commented Jul 4, 2017

@sammc ptal.

Web platform tests updated here: web-platform-tests/wpt@master...mgiuca:web-share-errors-and-arguments

index.html Outdated
<a data-cite=
"!WEBIDL#exceptiondef-typeerror"><code>TypeError</code></a> if called
with no arguments. WebIDL <a data-cite=
"!WEBIDL#idl-operations">mandates that it be optional</a>, because
Copy link

Choose a reason for hiding this comment

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

I don't think you want a comma here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

index.html Outdated
};
</pre>
<div class="note">
The <var>data</var> argument is marked as "optional", but really it
Copy link

Choose a reason for hiding this comment

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

The really feels a bit superfluous.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shane suggested I write a comment explaining why this is here, since it's quite unintuitive to have an optional argument that is mandatory.

In particular, I think while developers may not read the whole spec, the one thing that is fairly understandable to a non-web-spec person is IDL, so we may have confused developers reading the IDL and wondering why this is "optional". That's why I'm so strongly opposed to adding a misleading "optional" here, but if we are going to have it, I'd rather explain it.

If you'd like I can reduce the amount of text here and perhaps convert it into a // comment in the IDL itself rather than a note.

Copy link

Choose a reason for hiding this comment

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

I meant just the word "really." The rest is fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh lol, use quotes next time. (I read it as "This really feel a bit superfluous".)

index.html Outdated
};
</pre>
<div class="note">
The <var>data</var> argument is marked as "optional", but really it
Copy link

Choose a reason for hiding this comment

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

I meant just the word "really." The rest is fine.

This has no behavioural change, as passing an empty dictionary is a
TypeError. But it is required by the WebIDL spec.

Closes w3c#47.
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.

None yet

2 participants