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

Clarifications on fragids. See #50. #51

Merged
merged 1 commit into from
Jul 6, 2022
Merged

Clarifications on fragids. See #50. #51

merged 1 commit into from
Jul 6, 2022

Conversation

ioggstream
Copy link
Collaborator

@ioggstream ioggstream commented Jul 1, 2022

This PR

  • includes clarification on JSON Pointer and alias nodes compatibility
  • includes FAQ
  • fragment identifier moved from media type to section
  • empty fragment + / are both under JSON Pointer

@ioggstream
Copy link
Collaborator Author

cc: @cabo @handrews PTAL

@ioggstream ioggstream requested a review from eemeli July 1, 2022 11:35
Comment on lines 123 to 125
An empty fragment identifier reference the root
of the YAML representation graph.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems out of place here, esp. given the immediately preceding paragraph?

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem really is that this section is entirely out of place -- why is this in a different place from the JSON Pointer part?
I'm not so hot for entirely defining the JSON Pointer part in the media type template.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cabo here we define how to process fragment identifiers alias nodes: this is specific to YAML and can be reused by other specifications (e.g. xxx+yaml) that are not interested in JSON Pointer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@eemeli

This seems out of place here, esp. given the immediately preceding paragraph?

Do you think it's out of place because alias nodes cannot be empty? The point is that
fragment identification includes the ability to send an empty fragment, and in this case we need to specify
how to handle them.

Copy link
Contributor

@cabo cabo Jul 4, 2022

Choose a reason for hiding this comment

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

@cabo here we define how to process fragment identifiers alias nodes: this is specific to YAML and can be reused by other specifications (e.g. xxx+yaml) that are not interested in JSON Pointer.

First of all, I'm strongly against +yaml specs "not being interested in JSON Pointer".
+yaml should always include application/yaml fragment syntax (Liskov substitution is the whole point of sss).
But assuming that this is not so, why does an implementation have to be interested in anchors to get this essential JSON Pointer feature? (Empty JSON Pointer = entire tree.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ioggstream I have to agree with @cabo

You've explained why you did it this way, and I follow the steps in your reasoning. But I still find all of it very confusing and counter-intuitive compared to just saying outright that you support RFC 6901 JSON Pointer fragments. There's nothing stopping you from defining a fragment syntax that overlaps when it comes to the empty fragment, as long as the semantics are the same, but this weird carve-out-and-replacement makes things so much harder to understand.

Also agree with the Liskov substitution point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My original point here was that this sentence/paragraph is in a surprising place, as it's in a section which nominally "describes how to use alias nodes [...] as fragment identifiers". And while it's true that an empty fragment identifier references the root node, this seems like the wrong place to assert this.

However, given this continuing dialogue, I'm starting to veer towards leaving this whole complexity out of the spec, and following the application/json way of leaving fragment identifiers unspecified. This would allow/force +yaml suffixes to define their own way of dealing with them, exactly as they do currently with +json. I rather presume that nearly all real-world +yaml suffixes will also have a parallel +json suffix, and this would allow for parity between the two media types.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, given this continuing dialogue, I'm starting to veer towards leaving this whole complexity out of the spec, and following the application/json way of leaving fragment identifiers unspecified. This would allow/force +yaml suffixes to define their own way of dealing with them, exactly as they do currently with +json. I rather presume that nearly all real-world +yaml suffixes will also have a parallel +json suffix, and this would allow for parity between the two media types.

It is easier to simply do this right. JSON has this weird situation where JSON Pointer is the de-facto fragment identifier standard, but that hasn't been codified. Don't repeat that mistake.

Copy link
Contributor

Choose a reason for hiding this comment

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

(And, of course, there never will be parity between the two media types.
I'm not even sure what that would mean.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@cabo agreed, JSON's refusal to adopt the de-facto standard as an official standard is a huge pain. You end up having to specify things as URI + (non-fragment) JSON Pointer pairs in order to address part of a JSON resource without violating the standards.

  * reference media type section
  * thx to @cabo, @handrews for suggestions
@ioggstream
Copy link
Collaborator Author

According to @handrews this PR should better clarify JSON Pointer use, so I am going to merge it.

The decision on forcing +yaml fragment is deferred to #54 that I will rebase.

@ioggstream ioggstream merged commit d1e7c7d into main Jul 6, 2022
@ioggstream ioggstream deleted the ioggstream-50 branch July 6, 2022 08:47
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.

4 participants