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

Tweak async iterator algorithms #802

Merged
merged 3 commits into from Oct 16, 2019
Merged

Conversation

domenic
Copy link
Member

@domenic domenic commented Sep 24, 2019

Make both the "async iterator initialization steps" and the "get the next iteration result" take two arguments: the instance, and the async iterator. This ends up being more ergonomic than using the "current state" concept.


Preview | Diff

index.bs Outdated
The [=async iterator initialization steps=] for a <code class="idl">SessionManager</code>
async iterator |iterator| are:

1. Set |iterator|'s current state to "not yet started".
Copy link
Member

Choose a reason for hiding this comment

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

Should "current state" be xreffed like https://heycam.github.io/webidl/#environment-ready-promise ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's probably a good idea; I'll push that tweak tomorrow before merging.

@domenic domenic requested a review from Ms2ger September 25, 2019 06:49
@domenic domenic force-pushed the async-iterator-no-current-state branch from c14701b to 1ca64bd Compare September 25, 2019 06:49
@domenic
Copy link
Member Author

domenic commented Sep 25, 2019

@Ms2ger I force-pushed a revised commit. I realized I forgot to actually update the call sites of the algorithm, so the changes were more substantial. In particular I replaced the "state" definition with an "is finished" boolean in the ES binding. As such a new re-review would be appreciated.

@domenic
Copy link
Member Author

domenic commented Sep 25, 2019

I've also pushed a commit that allows "get the next iteration result" to reject to fix #803, since it was so small.

Make both the "async iterator initialization steps" and the "get the next iteration result" take two arguments: the instance, and the async iterator. This ends up being more ergonomic than using the "current state" concept.
@domenic
Copy link
Member Author

domenic commented Oct 11, 2019

@Ms2ger I'll be back at work next week, and was hoping you'd be able to review this and #805 before then. Ping :)

Copy link
Member

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

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

Seems fine

index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
* Export the terms we expect people to use
* Highlight the samples as Web IDL
* Clean up the description of the initialization steps
* Simplify the algorithm for next a bit
@domenic domenic force-pushed the async-iterator-no-current-state branch from 0af9c61 to 2d6954b Compare October 16, 2019 10:53
@domenic domenic merged commit 9d11c76 into master Oct 16, 2019
@domenic domenic deleted the async-iterator-no-current-state branch October 16, 2019 10:56
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.

None yet

2 participants