Skip to content

provide defaults for all keywords and subschemas that lacked them #1006

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.

Sign up for GitHub

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 7 commits into from
Jan 27, 2021
Merged

provide defaults for all keywords and subschemas that lacked them #1006

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9eef7bf
provide defaults for all keywords and subschemas that lacked them
karenetheridge Oct 22, 2020
7060428
remove redundant defaults adjacent to $dynamicRef
karenetheridge Jan 20, 2021
3ea4c02
remove misleading/incorrect defaults at top schema level
karenetheridge Jan 21, 2021
a33ef8f
squash - final (?) attempt - move defaults to where they belong, and …
karenetheridge Jan 21, 2021
b5ab09f
squash - these defaults do not make sense here - missing keywords pro…
karenetheridge Jan 21, 2021
5553c05
squash. remove defaults in the wrong place; add $comments for default…
karenetheridge Jan 22, 2021
677f20a
squash - okay, one last time. keep defaults at property level as huma…
karenetheridge Jan 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 5 additions & 10 deletions meta/applicator.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,28 +13,23 @@
"items": { "$dynamicRef": "#meta" },
"contains": { "$dynamicRef": "#meta" },
"additionalProperties": {
"$dynamicRef": "#meta",
"default": {}
"$dynamicRef": "#meta"
},
"properties": {
"type": "object",
"additionalProperties": { "$dynamicRef": "#meta" },
"default": {}
"additionalProperties": { "$dynamicRef": "#meta" }
},
"patternProperties": {
"type": "object",
"additionalProperties": { "$dynamicRef": "#meta" },
"propertyNames": { "format": "regex" },
"default": {}
"propertyNames": { "format": "regex" }
},
"dependentSchemas": {
"type": "object",
"additionalProperties": { "$dynamicRef": "#meta" },
"default": {}
"additionalProperties": { "$dynamicRef": "#meta" }
},
"propertyNames": {
"$dynamicRef": "#meta",
"default": {}
"$dynamicRef": "#meta"
},
"if": { "$dynamicRef": "#meta" },
"then": { "$dynamicRef": "#meta" },
Expand Down
6 changes: 4 additions & 2 deletions meta/core.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,12 @@
},
"$defs": {
"type": "object",
"additionalProperties": { "$dynamicRef": "#meta" },
"default": {}
"additionalProperties": { "$dynamicRef": "#meta" }
}
},
"default": {
"$schema": "https://json-schema.org/draft/2020-12/schema"
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this mean that every schema, including sub-schemas, have a $schema keyword. $schema has no effect in sub-schemas, so this doesn't make sense to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good point. So we'll have to leave it out.. as we have no way currently of specifying in the metaschema "this should apply for the top level schema but not for anything dynamically referenced lower down".

},
"$defs": {
"anchorString": {
"type": "string",
Expand Down
9 changes: 3 additions & 6 deletions meta/meta-data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,13 @@
},
"default": true,
"deprecated": {
"type": "boolean",
"default": false
"type": "boolean"
},
"readOnly": {
"type": "boolean",
"default": false
"type": "boolean"
},
"writeOnly": {
"type": "boolean",
"default": false
"type": "boolean"
},
"examples": {
"type": "array",
Expand Down
15 changes: 7 additions & 8 deletions meta/validation.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,11 @@
"maxItems": { "$ref": "#/$defs/nonNegativeInteger" },
"minItems": { "$ref": "#/$defs/nonNegativeIntegerDefault0" },
"uniqueItems": {
"type": "boolean",
"default": false
"type": "boolean"
},
"maxContains": { "$ref": "#/$defs/nonNegativeInteger" },
"minContains": {
"$ref": "#/$defs/nonNegativeInteger",
"default": 1
Copy link
Member

Choose a reason for hiding this comment

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

this should stay

Copy link
Member Author

Choose a reason for hiding this comment

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

we're okay with using "default" totally contradictory to the spec? If this is here purely for humans, it should be in a $comment.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this use is contradictory to the spec, because the spec defines default so broadly that there is no wrong way to use it. Since there is no right or wrong way to use it and it doesn't do anything anyway, I'd rather use it in the spirit of how it was intended to be used.

If this is here purely for humans, it should be in a $comment.

I had the same thought at first, but I've come to the conclusion that just because default is not useful as an annotation, doesn't mean it's not useful. Consider that a documentation generator doesn't use instances and therefore doesn't work on annotations as they are defined by the spec. The documentation generator can still make good use of default because the limitations of using it as an annotation don't apply in that context.

Copy link
Member

Choose a reason for hiding this comment

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

I'm in agreement with this. We should consider other applications beyond just being used with an instance, the most obvious, to me, being writing schemas by hand or using UI tools (in terms of adding these for the meta schema).

"$ref": "#/$defs/nonNegativeInteger"
},
"maxProperties": { "$ref": "#/$defs/nonNegativeInteger" },
"minProperties": { "$ref": "#/$defs/nonNegativeIntegerDefault0" },
Expand All @@ -68,14 +66,16 @@
}
}
},
"default": {
"minContains": 1
},
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of this format, and I don't think we should encourage it. I think the default value should stay with the property declaration.

"$defs": {
"nonNegativeInteger": {
"type": "integer",
"minimum": 0
},
"nonNegativeIntegerDefault0": {
"$ref": "#/$defs/nonNegativeInteger",
"default": 0
"$ref": "#/$defs/nonNegativeInteger"
},
"simpleTypes": {
"enum": [
Expand All @@ -91,8 +91,7 @@
"stringArray": {
"type": "array",
"items": { "type": "string" },
"uniqueItems": true,
"default": []
"uniqueItems": true
}
}
}
1 change: 0 additions & 1 deletion schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
"$comment": "\"definitions\" has been replaced by \"$defs\".",
"type": "object",
"additionalProperties": { "$dynamicRef": "#meta" },
"default": {},
"deprecated": true
},
"dependencies": {
Expand Down