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

Rename definitions to $defs in the 2019-06 draft. #266

Merged
merged 1 commit into from
Jun 30, 2019
Merged

Rename definitions to $defs in the 2019-06 draft. #266

merged 1 commit into from
Jun 30, 2019

Conversation

Julian
Copy link
Member

@Julian Julian commented Jun 15, 2019

As per https://json-schema.org/work-in-progress/WIP-jsonschema-validation.html#rfc.appendix.C

Refs #265

Although :( at

Renamed to "$defs" to match "$ref" and be shorter to type.

I don't like "shorter to type" as a reason to do anything personally :/ but hey that's as usual on me to say when noticing that change. Oh well.

There's also a few places in the WIP that still say definitions. Will find wherever that should be filed.

@Julian Julian requested review from handrews and gregsdennis June 15, 2019 20:27
@handrews
Copy link
Contributor

@Julian mostly we agreed that it should have a $ on it based on our taxonomy of keywords, and that was important enough to rename it. I wouldn't have shortened it on its own, but typing definitoins (note the order of the last two vowels) is seriously one of the most frustrating things about working with JSON Schema for me. I do it constantly and I'm sick of it, so since we had to change it anyway I decided to make life easier for myself. Perhaps the changelog entry should be less flippant, though.

@Julian
Copy link
Member Author

Julian commented Jun 16, 2019

typing definitoins (note the order of the last two vowels) is seriously one of the most frustrating things about working with JSON Schema for me. I do it constantly and I'm sick of it

:D haha aright, makes sense.

@@ -139,7 +135,7 @@
"$id": "http://localhost:1234/object",
"type": "object",
"properties": {
"name": {"$ref": "name.json#/definitions/orNull"}
"name": {"$ref": "name.json#/$defs/orNull"}
Copy link
Member

Choose a reason for hiding this comment

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

We may need a new /remotes/name.json:

{
    "definitions": {
        "orNull": {
            "anyOf": [
                {"type": "null"},
                {"$ref": "#"}
            ]
        }
    },
    "type": "string"
}

It has a definitions that 3 of the tests try to reference into.

  • refRemote
    • root in remote ref
      • null is valid
      • object is invalid
      • string is valid

This change updates the definitions to a $defs in the $refs causing the tests to (rightly) fail.

Copy link
Member

@gregsdennis gregsdennis Jun 17, 2019

Choose a reason for hiding this comment

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

Don't just change /remotes/name.json, though. We still need that for the other drafts. (I suppose you could add $defs with a copy of what's in definitions... ¯\_(ツ)_/¯ )

Copy link
Member Author

Choose a reason for hiding this comment

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

Ech, yes that sounds annoying to fix...

Copy link
Member

Choose a reason for hiding this comment

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

I actually tried to add $defs to name.json locally, but my implementation threw a fit because it couldn't determine which draft the schema was.

I have logic which analyzes the keywords to determine which drafts the schema conforms to if it's not specified explicitly by the $schema keyword. Since I don't list definitions as draft-8-compatible, having both it and $defs in the same schema gives it a sad.

I understand that definitions is included for backward compatibility, but I'm trying to dissuade people from using it by producing an error when it is used in a draft-8 schema.

In the end, I ended up creating a name-draft-8 file to get it passing locally.

@gregsdennis
Copy link
Member

gregsdennis commented Jun 17, 2019

There are also several places that refer to http://json-schema.org/draft-07/schema# that need to be updated to something... http://json-schema.org/draft-08/schema#? http://json-schema.org/draft-2019-06/schema#?

Interestingly, these places are causing definitions to pop up in the test results, e.g.

...
{
  "valid" : true,
  "keywordLocation" : "#/$ref/definitions",
  "absoluteKeywordLocation" : "http://json-schema.org/draft-07/schema#/definitions",
  "instanceLocation" : "#",
  "keyword" : "definitions"
},
...

@Julian
Copy link
Member Author

Julian commented Jun 19, 2019

The canonical URI thing sounds like something @Relequestual / @handrews should have opinions on? Whatever we are choosing yeah should go there.

@Relequestual
Copy link
Member

We decided that meta-schema file names would follow DATE as opposed to draft numbers.
json-schema-org/json-schema-spec#612
I think it may cause confusion, but less so than having multiple published versions using the same identifier, which is what we get if we simply publish a fix.

This is covered in the core spec: https://json-schema.org/work-in-progress/WIP-jsonschema-core.html#rfc.section.7.5 (WIP URL will vanish, so here's the extract as of today.)

Updated vocabulary and meta-schema URIs MAY be published between specification drafts in order to correct errors. Implementations SHOULD consider URIs dated after this specification draft and before the next to indicate the same syntax and semantics as those listed here.

The date should be 2019-06 as according to our set out schedule, publication will happen tomorrow (but this relies more on @handrews availability, and we shouldn't hold fast to a specific date. We should remain flexible given Henrys heroics and current personal circumstances!)

@gregsdennis gregsdennis mentioned this pull request Jun 29, 2019
Julian added a commit that referenced this pull request Jun 30, 2019
Even though we haven't done that elsewhere yet (see #266, #267)
might as well refrain from adding new ones...
@Julian Julian merged commit 65e3480 into master Jun 30, 2019
@Julian Julian deleted the defs branch June 30, 2019 21:04
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