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 AccessHandles to spec #344

Closed
wants to merge 19 commits into from
Closed

Add AccessHandles to spec #344

wants to merge 19 commits into from

Conversation

fivedots
Copy link
Collaborator

@fivedots fivedots commented Nov 3, 2021

This draft PR adds AccessHandles to the spec. It's currently missing the content on all AccessHandle methods, as well as the logic that releases the lock once a WritableStream is closed.


Preview | Diff


Preview | Diff

@fivedots
Copy link
Collaborator Author

fivedots commented Nov 3, 2021

Hi @mkruisselbrink , could you PTAL? It's still a draft, but some early feedback would be great!

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
fivedots and others added 3 commits November 23, 2021 00:58
Co-authored-by: Thomas Steiner <tomac@google.com>
Co-authored-by: Thomas Steiner <tomac@google.com>
Co-authored-by: Thomas Steiner <tomac@google.com>
@annevk
Copy link

annevk commented Feb 11, 2022

Could you rebase these commits against https://github.com/whatwg/fs and create the PR there? I'm willing to help with review.

fivedots and others added 5 commits March 1, 2022 00:44
This PR removes the naming open question and updates the table of contents. Lately there hasn't been an active discussion on naming, nor consensus on a better scheme, so it seems like the right time to remove it in preparation for shipment.
* Fix drag and drop example

Fixes #356

* Update index.bs
@fivedots
Copy link
Collaborator Author

@annevk will do after filling in the last couple missing methods, thanks for the review offer!

Copy link
Contributor

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

Overall this looks really good. You have exactly the right spirit and approach to rigorously specifying things.

I left a ton of comments, but only a few of them are substantative (and I marked them as "Important"). The vast majority are about style issues.

And, I bet a lot of those style issues are preexisting ones with the spec, so if you followed my suggestions you'd be introducing some inconsistency. In that case, you should feel free to not fix them here, but instead file issues (either on whatwg/fs or on this repo) to clean them up later, at a lower priority.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@fivedots fivedots mentioned this pull request Apr 11, 2022
3 tasks
@fivedots
Copy link
Collaborator Author

I'm closing this PR in favor of continuing the review in whatwg/fs#21, please leave any feedback there!

@fivedots fivedots closed this Apr 12, 2022
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.

4 participants