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

Editorial: quick fixes for recent merges #2129

Merged
merged 3 commits into from
Aug 11, 2020
Merged

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Aug 9, 2020

No description provided.

1. Set _r_.[[Key]] to ~empty~.
1. Set _r_.[[Value]] to ~empty~.
1. For each WeakSet _set_ such that _set_.[[WeakSetData]] contains _obj_, do
1. Replace the element of _set_ whose value is _obj_ with an element whose value is ~empty~.
1. Replace the element of _set_.[[WeakSetData]] whose value is _obj_ with an element whose value is ~empty~.
Copy link
Member

Choose a reason for hiding this comment

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

nice catch, this seems like an actual spec bug

spec.html Show resolved Hide resolved
@ljharb ljharb added the spec bug label Aug 9, 2020
@ljharb ljharb requested review from michaelficarra, syg, bakkot and a team August 9, 2020 03:00
@ljharb ljharb self-assigned this Aug 9, 2020
@ljharb ljharb requested a review from a team August 9, 2020 20:44
@ljharb
Copy link
Member

ljharb commented Aug 10, 2020

@jmdyck do you think these commits could/should be combined, or do you think it's best if they remain separate?

@jmdyck
Copy link
Collaborator Author

jmdyck commented Aug 10, 2020

Generally I think it's best to keep commits separate when they perform independent changes, but in this case, if you combined them (and their commit messages), I think the result would be okay.

If the .[[WeakSetData]] change fixed a spec bug, that should maybe be a separate commit.

Another possibility would be to combine commits that relate to the same PR-merge. (4 are for 2089, the last is for 1948)

I can split/combine as you like.

@ljharb
Copy link
Member

ljharb commented Aug 10, 2020

@jmdyck i think it makes sense to have one commit for each PR that's being fixed; but to also keep the spec bug as a separate one - so maybe 3 total? That'd be great, thanks :-)

@jmdyck
Copy link
Collaborator Author

jmdyck commented Aug 11, 2020

Done!

@jmdyck
Copy link
Collaborator Author

jmdyck commented Aug 11, 2020

(repushed to put the middle commit's points in a better order)

jmdyck added 3 commits August 10, 2020 23:14
- Use title-case in clause heading

- Tweak some algorithm syntax

- Put 2 emu-clause attributes in quotes
  (We always quote element attributes -- see PR tc39#1868)

- Change "a finalization registry object" to "a FinalizationRegistry"
  (The former term isn't defined.)
@ljharb ljharb merged commit f3455c2 into tc39:master Aug 11, 2020
h2oche pushed a commit to h2oche/ecma262 that referenced this pull request Aug 16, 2020
h2oche pushed a commit to h2oche/ecma262 that referenced this pull request Aug 16, 2020
- Use title-case in clause heading

- Tweak some algorithm syntax

- Put 2 emu-clause attributes in quotes
  (We always quote element attributes -- see PR tc39#1868)

- Change "a finalization registry object" to "a FinalizationRegistry"
  (The former term isn't defined.)
h2oche pushed a commit to h2oche/ecma262 that referenced this pull request Aug 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants