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

Resolve the questions about "JSON Serialization" term #608

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

anatoly-scherbakov
Copy link

@anatoly-scherbakov anatoly-scherbakov commented Jul 23, 2024

Fixes #605.


💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on Dec 4, 2024, 7:17 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.

🔗 Related URL

Timed out after waiting 30000ms

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@gkellogg
Copy link
Member

We would need to define what the "Object Representation" is, and really, it's not a representation of the Internal Representation, it is the Internal Representation.

The first two substitutions are actually the JSON representation, so those should remain unchanged.

@anatoly-scherbakov
Copy link
Author

@gkellogg what about this?

  • "Object Representation" removed;
  • Notes about JSON-LD API functions converting their output from JSON Serialization removed.

@anatoly-scherbakov
Copy link
Author

Might close: #605

index.html Outdated Show resolved Hide resolved
@anatoly-scherbakov anatoly-scherbakov changed the title JSON Serialization → Object Representation Resolve the questions about "JSON Serialization" term Jul 24, 2024
@anatoly-scherbakov anatoly-scherbakov force-pushed the 605-json-serialization branch 2 times, most recently from 8b2db03 to 5f9b388 Compare August 3, 2024 17:02
@anatoly-scherbakov
Copy link
Author

Added two subsections trying to explain these things. @gkellogg what do you think?

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link
Member

@gkellogg gkellogg left a comment

Choose a reason for hiding this comment

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

In addition, I think 9.1 should have a paragraph referencing Serialization and Deserialization in expand() step 3. It should also have a general statement about serializing the results of an API method as described in Serialization and Deserialization or some other representation, based on the requested format type.

See additional inline suggestions.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated
Comment on lines 6142 to 6182
<dt>
To save a JSON-LD document in <a>internal representation</a>
to a file,
</dt>
<dd>
a user of an implementation has to serialize that data structure
to JSON;
</dd>
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 a bit awkward, as when it's in the internal representation, it's not a JSON-LD document.

Perhaps something more like the following:

To serialize the internal representation to JSON-LD, an implementation serializes that representation to a suitable JSON representation.

Copy link
Member

Choose a reason for hiding this comment

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

That seems somewhat circular... What would be a suitable JSON representation except JSON-LD?

Copy link
Member

Choose a reason for hiding this comment

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

Should be a "suitable representation", which can include JSON and YAML, but this spec only talks about the JSON variant of that encoding.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe --

To serialize the internal representation to JSON-LD, an implementation first serializes that internal representation to a suitable external representation, such as JSON or YAML.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for a late reply.

I thought the point is that a JSON-LD implementation, or YAML-LD implementation, is not required to be able to write a JSON or a YAML string or file.

There's no such requirement in the spec, at least. Writing JSON/YAML files or converting Internal Representation to strings is left for the user to manage as they see fit.

Is that not right?

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@gkellogg
Copy link
Member

We'll need a normative statement (in an appropriately normative section) that a processor MUST serialize the internal representation to JSON when responding to a content negotiated request for application/json or a sub-type (such as application/ld+json). YAMl-LD would need a similar statement, as would CBOR-LD. The section 9.1.2 Serialization and Deserialization is informative sets the stage for other format serialization requirements.

@anatoly-scherbakov
Copy link
Author

@gkellogg

processor MUST serialize the internal representation to JSON when responding to a content negotiated request for application/json or a sub-type (such as application/ld+json)

I see but... I think processor does not handle content negotiation at all, does it? That's what the user application can be doing. Or not. For instance, I write a console program which reads and writes JSON-LD and YAML-LD; it has nothing to do with content negotiation. But it serializes data in either format based on what --format value I submit to it when calling.

@gkellogg
Copy link
Member

gkellogg commented Sep 3, 2024

@gkellogg

processor MUST serialize the internal representation to JSON when responding to a content negotiated request for application/json or a sub-type (such as application/ld+json)

I see but... I think processor does not handle content negotiation at all, does it? That's what the user application can be doing. Or not. For instance, I write a console program which reads and writes JSON-LD and YAML-LD; it has nothing to do with content negotiation. But it serializes data in either format based on what --format value I submit to it when calling.

There are plenty of normative requirements about using HTTP headers, such as Accept, Link and Profile. A JSON-LD processor MUST return a suitable serialization based on the Accept header (content negotiation). If the header indicates application/yaml or a sub-class, that would be YAML, if application/json or a sub-class, that would be JSON. We previously (erroneously) did this in the API method descriptions. The idea is to make the wording more generic and make the serialization requirement not directly tied to the detailed descriptions of the API methods.

@anatoly-scherbakov
Copy link
Author

@gkellogg maybe you could please direct me to tests covering this behavior?

@gkellogg
Copy link
Member

gkellogg commented Sep 3, 2024

You want the Remote Document Test Manifest.

@anatoly-scherbakov
Copy link
Author

@gkellogg I believe the deserialization procedure is handled by LoadDocumentCallback, adding a reference to it at the Serialization and Deserialization section. I am not sure how to go about the serialization though. Perhaps let's discuss this on the nearest meeting.

@gkellogg gkellogg added the class-3 Class-3 change label Oct 19, 2024
@pchampin
Copy link
Contributor

This was discussed during the json-ld meeting on 13 November 2024.

View the transcript

w3c/json-ld-api#608

<gb> Pull Request 608 Resolve the questions about "JSON Serialization" term (by anatoly-scherbakov) [spec:substantive] [ErratumRaised] [class-3]

gkellogg: once we have approvals, we can merge.

bigbluehat: There are more issues to discuss, but we'll continue to just move through the list. Cleaning house before worrying about the charter makes sense.

<bigbluehat> https://github.com/orgs/w3c/projects/84

bigbluehat: The project manager should include YAML-LD, CBOR-LD, JSON-LD-star and other things the WG is working on.


Copy link
Contributor

@pchampin pchampin left a comment

Choose a reason for hiding this comment

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

I like the general direction of this PR, but there are few places where I find the wording confusing or slightly inaccurate (see my comments)

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
anatoly-scherbakov and others added 8 commits November 29, 2024 23:42
Co-authored-by: Pierre-Antoine Champin <github-100614@champin.net>
Co-authored-by: Pierre-Antoine Champin <github-100614@champin.net>
Co-authored-by: Pierre-Antoine Champin <github-100614@champin.net>
Co-authored-by: Pierre-Antoine Champin <github-100614@champin.net>
Co-authored-by: Pierre-Antoine Champin <github-100614@champin.net>
Co-authored-by: Pierre-Antoine Champin <github-100614@champin.net>
Co-authored-by: Pierre-Antoine Champin <github-100614@champin.net>
Co-authored-by: Pierre-Antoine Champin <github-100614@champin.net>
@gkellogg
Copy link
Member

@anatoly-scherbakov when you have several suggestions to commit, try using the batch commit feature from the Files menu; it creates only one commit which includes all the suggestions you add.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link
Contributor

@pchampin pchampin left a comment

Choose a reason for hiding this comment

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

a few more non-controversial changes (mostly due to mistakes in my suggested changes 😅), but LGTM

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Thanks!

Co-authored-by: Pierre-Antoine Champin <github-100614@champin.net>
@anatoly-scherbakov
Copy link
Author

Thanks!

I don't think I can merge:

You're not authorized to push to this branch. Visit https://docs.github.com/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches for more information.

Copy link
Member

@gkellogg gkellogg left a comment

Choose a reason for hiding this comment

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

If these are class-3 changes (which I believe they are), we can't simply merge this. It needs to be marked up with <ins>/<del> similar to the other class-3 changes, and have an appropriate entry in the "Changes since REC" section.

It's slightly more complicated to add these changes to a remote repository, but I can do this if you're not comfortable with it.

index.html Outdated Show resolved Hide resolved
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
@anatoly-scherbakov
Copy link
Author

@gkellogg I am open to doing it. Perhaps you could suggest where to look for an example, maybe another, already merged, PR formulated in this way?

I will open further PRs in the main repo rather than my fork.

@gkellogg
Copy link
Member

gkellogg commented Dec 7, 2024

@gkellogg I am open to doing it. Perhaps you could suggest where to look for an example, maybe another, already merged, PR formulated in this way?

Check out #616 for how it is done. Basically, a div with class "candidate correction" and an id of "change_5" (or whatever is the next correction in sequence) such as is found here

json-ld-api/index.html

Lines 1637 to 1641 in c05d6af

<div class="candidate correction" id="change_3">
<span class="marker">Candidate Correction 3</span>
<p>This changes the algorithm for processing a reverse term so that it doesn't return early.
For more information, refer to <a href="https://github.com/w3c/json-ld-api/issues/565">issue 565</a>.</p>
</div>
.

And ins del with citations to the correction such as

json-ld-api/index.html

Lines 1678 to 1679 in c05d6af

<li><ins cite="#change_3">Otherwise, if</ins><del cite="#change_3">If</del> <var>value</var> contains the <a>entry</a> <code>@id</code> and its value
does not equal <var>term</var>:

Also, an entry in the Changes since Rec referencing the issue and the candidate correction div.

It's more complicated when there are changes in multiple places. There, you might have links to each correction ins/del from the candidate correction block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

It is not clear enough what "JSON Serialization" is
4 participants