Skip to content

Add "$comment" as a keyword #359

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

Merged
merged 3 commits into from
Sep 5, 2017
Merged

Conversation

handrews
Copy link
Contributor

This addresses issue #197. The primary purpose is to reserve the
keyword and ensure that implementations do NOT do anything based
on its presence or contents. This improves interoperability by
ensuring that comments are safe for use with any conforming system.

Without this keyword, two implementations may implement it with
differing semantics, potentially producing undesirable or
insecure behavior.

Effort has been made to ensure that existing implementations
that properly ignore unknown keywords will not need to make
any changes at all to be in conformance for this keyword.

I noticed while adding $comment that keywords are always quoted
in section headings unless the keyword is the entire heading.
This is true across all three specifications, so the trivial
second commit brings the only outlier ($ref) in line with everything else.

This addresses issue json-schema-org#197.  The primary purpose is to reserve the
keyword and ensure that implementations do NOT do anything based
on its presence or contents.  This improves interoperability by
ensuring that comments are safe for use with any conforming system.

Without this keyword, two implementations may implement it with
differing semantics, potentially producing undesirable or
insecure behavior.

Effort has been made to ensure that existing implementations
that properly ignore unknown keywords will not need to make
any changes at all to be in conformance for this keyword.
I noticed while adding $comment that keywords are always quoted
in section headings unless the keyword is the entire heading.
This is true across all three specifications, so this trivial
change brings the only outlier in line with everything else.
@handrews handrews added this to the draft-07 (wright-*-02) milestone Aug 21, 2017
Copy link
Member

@dlax dlax left a comment

Choose a reason for hiding this comment

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

Just spotted a typo. LGTM otherwise.

specification.

Tools that translate other media types or programming languages
to and from application/schema+json MAY choose convert that media type or
Copy link
Member

Choose a reason for hiding this comment

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

MAY choose to convert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks!

This ensures that comments can be used in structures such as
Hyper-Schema's LDOs without every specification needing to
define it separately.
@handrews
Copy link
Contributor Author

handrews commented Sep 3, 2017

Last call- this will get merged tomorrow at the 14 day mark barring a compelling reason to reconsider.

@Julian
Copy link
Member

Julian commented Sep 3, 2017

I am not a huge fan of JSON Schema attempting to step in for JSON's shortcomings. But I won't pretend to have "compelling" reason not to add this, so I suppose I'm just a -0.

@handrews
Copy link
Contributor Author

handrews commented Sep 3, 2017

@Julian yeah, I think that objection was pretty solidly covered in the issue. The implementation requirements for this are literally "don't do anything" and many schema authors view it as very helpful (based on usage in teams that I have worked with, and/or reasons given for not working with JSON Schema unless allowed to use something other than JSON).

@handrews
Copy link
Contributor Author

handrews commented Sep 3, 2017

It also does not attempt to be a generic comment mechanism for JSON, nor would I support it as such. Its syntax and rationale are very specific to the structure of JSON Schema, and working with JSON Schema's extensible nature and the likelihood of problems caused by clashing extensions in this area.

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