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: pass necessary arguments to SerializeJSON* #1890

Merged
merged 1 commit into from Mar 13, 2020
Merged

Editorial: pass necessary arguments to SerializeJSON* #1890

merged 1 commit into from Mar 13, 2020

Conversation

@bakkot
Copy link
Contributor

bakkot commented Mar 6, 2020

SerializeJSONProperty , SerializeJSONObject, and SerializeJSONArray refer to variables which they have not been passed using notation like SerializeJSONProperty has access to _ReplacerFunction_ from the invocation of the `stringify` method.

I find this sketchy (see #1884).

This PR makes all the necessary values be passed explicitly, wrapped up in a Record.

@ljharb ljharb requested review from michaelficarra, syg, ljharb and tc39/ecma262-editors Mar 6, 2020
@ljharb
ljharb approved these changes Mar 6, 2020
Copy link
Member

ljharb left a comment

suggestions optional

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@ljharb ljharb self-assigned this Mar 6, 2020
@bakkot

This comment has been minimized.

Copy link
Contributor Author

bakkot commented Mar 6, 2020

Hm. Asserting that something is a Record isn't all that useful, since that tells you nothing about the fields in that record, which are just as important. So I think I'd prefer to omit those assertions, though it's not a strong preference.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 6, 2020

In general I prefer abstract ops to assert about their arguments as strongly as possible; certainly we could make a JSONStateRecord type and assert it's one of those (which would include the slots), but that feels a bit overkill here.

@bakkot

This comment has been minimized.

Copy link
Contributor Author

bakkot commented Mar 6, 2020

I agree that it feels like overkill to make such an assertion, but at that point I don't really see the value of making any assertion at all. There's nowhere else in the spec we just assert something is a Record, as far as I know.

@syg
syg approved these changes Mar 13, 2020
Copy link
Contributor

syg left a comment

Nice cleanup, lgtm.

spec.html Outdated Show resolved Hide resolved
@ljharb ljharb force-pushed the json-state branch from cf3d3b5 to 94d8aff Mar 13, 2020
@ljharb ljharb force-pushed the json-state branch from 94d8aff to 2b8561c Mar 13, 2020
@ljharb ljharb merged commit 2b8561c into master Mar 13, 2020
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
netlify/ecma262-snapshots/deploy-preview Deploy preview ready!
Details
@ljharb ljharb deleted the json-state branch Mar 13, 2020
ljharb added a commit to jmdyck/ecma262 that referenced this pull request Mar 21, 2020
(re tc39#1890)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.