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

Serialize JSON by reference to Infra spec #465

Merged
merged 7 commits into from
Mar 10, 2021

Conversation

sideshowbarker
Copy link
Contributor

@sideshowbarker sideshowbarker commented Feb 18, 2021

Fixes #455


Preview | Diff

@annevk
Copy link
Member

annevk commented Feb 18, 2021

You will also need to change

Let |body| be a new JavaScript object with properties initialized as follows:

to be an Infra map or some such.

(And you need to run make.)

@mikewest
Copy link
Member

To Anne's point, I think that the example https://infra.spec.whatwg.org/#example-map-notation is basically what we'd want to copy/paste into that step.

@sideshowbarker
Copy link
Contributor Author

To Anne's point, I think that the example infra.spec.whatwg.org/#example-map-notation is basically what we'd want to copy/paste into that step.

Thanks, yep — will update the patch with that addition

@sideshowbarker
Copy link
Contributor Author

sideshowbarker commented Feb 19, 2021

You will also need to change

Let |body| be a new JavaScript object with properties initialized as follows:

to be an Infra map or some such.

OK, changed to that.

@sideshowbarker sideshowbarker force-pushed the sideshowbarker/json-serialize-reference-infra branch from a2ff90f to 9c41baa Compare February 19, 2021 05:47
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Nice!

index.src.html Outdated

4. Let |object| be a new JavaScript object with properties initialized as
4. Let |object| be a <a lt="ordered map">map</span> with a single key initialized as
follows:

: "`csp-report`"
:: |body|
Copy link
Member

Choose a reason for hiding this comment

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

This is the perfect opportunity to use

Let object be «[ "csp-report" → body

I suggest we go for it. (Or even inline that into the next step.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the perfect opportunity to use

Let object be «[ "csp-report" → body ]»

I suggest we go for it. (Or even inline that into the next step.)

OK, thanks — I went all the way and inlined it. I like it. One less step. But if it’s too far and we think it’d be more clear to do the |object|-indirection thing, I’m happy to do it that way instead.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to start normalizing this.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I noticed some weird formatting when looking at the preview. I guess validation might not be CI-checked either...

index.src.html Outdated Show resolved Hide resolved
@sideshowbarker
Copy link
Contributor Author

I noticed some weird formatting when looking at the preview.

oops — thanks for catching that

I guess validation might not be CI-checked either...

Right. But I can add it to the GitHub Actions workflow, after we get the initial deploy part landed

@sideshowbarker sideshowbarker force-pushed the sideshowbarker/json-serialize-reference-infra branch from 388fb98 to 47c5dee Compare March 10, 2021 17:51
@sideshowbarker sideshowbarker merged commit 78d0908 into main Mar 10, 2021
@sideshowbarker sideshowbarker deleted the sideshowbarker/json-serialize-reference-infra branch March 10, 2021 17:52
github-actions bot added a commit that referenced this pull request Mar 10, 2021
…thored-by: Anne van Kesteren <annevk@annevk.nl>

SHA: 78d0908
Reason: push, by @sideshowbarker

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

Use Infra for JSON
3 participants