Skip to content

More fixes for undefined #965

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

Merged
merged 7 commits into from
Apr 22, 2021
Merged

More fixes for undefined #965

merged 7 commits into from
Apr 22, 2021

Conversation

tabatkins
Copy link
Contributor

  • Added undefined to the distinguishability table
  • Removed text from any/union conversion that handled undefined specially, now that it's just an ordinary IDL value.
  • Editorially, cleaned up the markup for the distinguishability table, and changed one unrelated code example to use double-quotes on a few strings, as it was inconsistent with quote usage elsewhere in the same example, and the empty string messed up Bikeshed's syntax-highlighting for the rest of the spec from that point on.

Currently a draft because there's an open question in #962 (comment), don't merge until that's answered and likely has another commit addressing it.

@tabatkins tabatkins marked this pull request as ready for review March 15, 2021 22:57
@tabatkins
Copy link
Contributor Author

tabatkins commented Mar 15, 2021

Ready for review now!

Based on discussion in #962:

  • undefined is not distinguishable from dictionary types; (undefined or MyDict) is invalid (because all dictionary types are implicitly optional and thus already include undefined in their "true" type)
  • undefined is nullable; undefined? is valid (will return an IDL null for a JS null, or IDL undefined for anything else)
  • undefined is technically implicitly nullable, since it converts anything into itself, per previous spec text and void behavior

Comment on lines +3608 to +3622
<thead>
<tr>
<th class="corner"></th>
<th><div><span>undefined</span></div>
<th><div><span>boolean</span></div>
<th><div><span>numeric types</span></div>
<th><div><span>bigint</span></div>
<th><div><span>string types</span></div>
<th><div><span>object</span></div>
<th><div><span>symbol</span></div>
<th><div><span>interface-like</span></div>
<th><div><span>callback function</span></div>
<th><div><span>dictionary-like</span></div>
<th><div><span>sequence-like</span></div>
</thead>
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that every other table in this specification includes the closing </t[d|h|r]> tags.

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 mean, I'm happy to remove those from all the other tables too, it just wasn't relevant for this PR. I can do it in a followup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, doing it in a separate PR would make this diff cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to close the loop: are you saying that removing the reformatting of this table is a requested change for an R+ here? Or is it just a drive-by preference, and a commitment to filing a clean-up PR afterwards reformatting the rest of the tables accordingly would be sufficient?

Copy link
Contributor

@ExE-Boss ExE-Boss Apr 18, 2021

Choose a reason for hiding this comment

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

It’s mainly a drive‑by preference for cleaner diffs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, can you r+? Or else tell me that you won't r+ until I make the change?

Copy link
Member

Choose a reason for hiding this comment

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

There might be some confusion here; @ExE-Boss is not an editor for this spec and is just doing a drive-by review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhhh, the GH "Reviewer" UI is confusing, sorry about that. In that case could I get a review, Domenic? ^_^

Co-authored-by: ExE Boss <[email protected]>
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Sorry this has been lagging so long. This is really great work in all respects. Undefined being nullable is a little wonky but seems harmless.

@domenic domenic merged commit ecfa7fb into whatwg:master Apr 22, 2021
@tabatkins tabatkins deleted the more-undefined branch April 29, 2022 20:07
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.

3 participants