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

Reviewers for Stage 3 #17

Open
3 of 7 tasks
leobalter opened this issue May 13, 2021 · 11 comments
Open
3 of 7 tasks

Reviewers for Stage 3 #17

leobalter opened this issue May 13, 2021 · 11 comments

Comments

@leobalter
Copy link
Member

leobalter commented May 13, 2021

Tracking issue for spec reviews for advancement to Stage 3.

From notes: JHD, RGN, BSH, DE MAH

@ljharb
Copy link
Member

ljharb commented May 14, 2021

I'm still not a fan of the term "identity" here - I believe tc39/ecma262#2405 specifically intends to only use that term with spec values and not ecmascript values.

Otherwise, the only nit I have is that HasIdentity could say "is Object or Symbol," and be reduced to two lines (none of these should be stage 3 blockers, but I'd strongly advocate for changing the word to something less confusing before merging into the main spec)

@littledan
Copy link
Member

LGTM for Stage 3. I think we can iterate on the names of abstract algorithms after Stage 3.

@brad4d
Copy link

brad4d commented May 18, 2021

LGTM for Stage 3 modulo concerns from @ljharb
Thanks for pointing those out BTW.

@leobalter
Copy link
Member Author

Thanks! I have no strong preference over the abstraction name. HasIdentity was called this way to reflect general functionality but I also don't intend to expand this proposal to other types.

One review item I received in private:

we can't have this be a slippery slope and add numbers and booleans to weak collections

I don't have intention or goals to add other primitive values to weak collections and this remains out of scope for this proposal. Someone else might wanna discuss about Records and Tuples but it's not the case for this proposal.

@leobalter
Copy link
Member Author

Thanks! @ljharb @brad4d #18 should address most of it. I'm now using the most simple name for the abstraction and I hope we are good to go with it.

@acutmore
Copy link
Contributor

acutmore commented May 16, 2022

Almost 1 year later, we are once again in a position where we require spec review as we prepare to go for stage 3.

As per #19 the full set of proposed changes is being done as a PR due to the proposal's need for many small changes (e.g. updating liveness to reference both objects and symbols).

acutmore/ecma262#1

The key difference from last year is that 'registered symbols' (those created with Symbol.for) are not considered suitable weak keys/entries/targets.

cc: @mhofman @gibson042 @brad4d @ljharb @michaelficarra @bakkot @syg

@leobalter
Copy link
Member Author

@acutmore Thanks for driving this! One pet peeve: we'd all benefit of a direct link to the rendered spec, but I'm also in favor of having this mentioned PR merged already for the said review.

@acutmore
Copy link
Contributor

@acutmore Thanks for driving this! One pet peeve: we'd all benefit of a direct link to the rendered spec, but I'm also in favor of having this mentioned PR merged already for the said review.

No worries! Good idea - I've switched things around so we now get a rendered preview here: https://arai-a.github.io/ecma262-compare/?pr=2777

@ljharb
Copy link
Member

ljharb commented May 31, 2022

Updated review (looking at the 262 PR)

@brad4d
Copy link

brad4d commented May 31, 2022

lgtm for stage 3

@rricard
Copy link
Member

rricard commented Jun 6, 2022

Hi @gibson042 & @ljharb I marked you both as reviewed here as Ashley did go through your comments. Let me know if that is not satisfying to you, I can rescind that review.

@mhofman can you let us know if you had a chance to review?

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

No branches or pull requests

10 participants