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

Add the "example" keyword. #123

Merged
merged 1 commit into from
Nov 21, 2016
Merged

Conversation

handrews
Copy link
Contributor

@handrews handrews commented Nov 3, 2016

This addresses issue #121.

Most of the wording is lifted from "default", which is similar and can serve
the same purpose. This wording explicitly allows for using "default" as an
example which was not part of the original request but seems reasonable.

Obviously, that part can be removed if deemed undesirable.

@epoberezkin
Copy link
Member

I think default and example should be separate - that follows from the usage practice. Defaults are used to populate missing properties (instead of failing) and example seems likely to be used on the top level as the example of the data that should be valid. I would specify that example MUST be valid according to the schema.

@handrews
Copy link
Contributor Author

handrews commented Nov 3, 2016

I feel like insisting that they be separate will lead to a lot of

{
    "type": "number",
    "default": 0,
    "example": 0
}

I kept the RECOMMENDED language from default instead of MUST because a) it made example compatible with default and b) because this way you could write something more elaborate for your example without breaking the schema.

@epoberezkin
Copy link
Member

I don't think so, the use case is different... Default is for missing data to be supplied. Example is purely informational.

@handrews
Copy link
Contributor Author

handrews commented Nov 3, 2016

I don't think so, the use case is different... Default is for missing data to be supplied. Example is purely informational.

You're just repeating your argument. The example is purely informational, and is never used as a default. However, the default value is a reasonable value to use to show an example in the absence of an explicit example.

I'm not dead set on this, but I'd like to hear at least one more opinion. @geemus this was originally your issue- thoughts?

@geemus
Copy link
Collaborator

geemus commented Nov 3, 2016

Hey, nice to see progress!

Lining this up with default seems pretty reasonable. That said, I am surprised that default is RECOMMENDED valid, as this could lead to non-validating data unless you specified something. It seems reasonable that the two would match one another though, for consistency.

In absence of an explicit example, having a MAY fallback to default also seems reasonable to me. It might not be the best example in certain cases, but it should hopefully be a valid example nonetheless. It would also allow schemas that don't need to specify a distinct example to be less verbose and/or redundant as @handrews suggests. That said, as mentioned above, the RECOMMENDED valid makes this a bit more problematic/troubling, as an in-valid example is probably worse than no example at all.

@handrews
Copy link
Contributor Author

handrews commented Nov 3, 2016

Let's separate the concerns over RECOMMENDED from the concerns over being able to use the default as an example. It's weird, but it's pre-existing weird and if we change it for default then we'd change example to match. Would you like to open an issue for that? BTW I see it as "RECOMMENDED" because if you have a sufficiently complex schema involving "oneOf", it might be rather complicated to be sure that a similarly complex default always validates. I admit, I'm reaching a bit here trying to justify this- I don't know the original rationale.

@epoberezkin : as @geemus notes, using a default as an example is a "MAY" thing. Implementations can choose to do it if they want, but it imposes no requirement to do so.

I wrote it like that because I already see systems using a default as an example. Writing it to forbid that seems like a regression. This just explicitly allows for continuing the behavior that already exists.

@geemus
Copy link
Collaborator

geemus commented Nov 3, 2016

Sounds reasonable to me. I've opened #125 to track the may vs must.

@jdesrosiers
Copy link
Member

The original rationale for default not being required is that it is not intended to be involved in validation. default is a metadata keyword. It is RECOMMENDED that you don't use a value that would confuse your users because that would be stupid, but the validation spec doesn't care in the slightest what the value is. The language in the spec is there if implementations want to do something special with default. But any additional constraints that need to be imposed on the default keyword to do that special thing is up to the implementation to define, not the validation spec.

A JSON Schema UI spec, on the other hand, would likely make use of default beyond meta-data. That is where an additional constraint on default should be added (if necessary). In fact, when/if a JSON Schema UI spec is created, I think default should be be moved there.

@handrews
Copy link
Contributor Author

handrews commented Nov 4, 2016

I'm strongly against taking default out of the main JSON Schema project. I have never used JSON Schema for anything remotely resembling a UI, but I have needed to use default a great deal.

@Anthropic
Copy link
Collaborator

I see the JSON UI Schema spec allowing overrides of current JSON Schema properties, So I would suggest it be considered when determining how strict the language is applied to the JSON Schema spec. Otherwise any keyword being overridden would require a complete and potentially conflicting re-definition in JSON UI Schema, which I don't see as ideal.

@awwright
Copy link
Member

awwright commented Nov 4, 2016

REQUIRED or else... what? MUST/REQUIRED proscribes behavior that an implementation must follow. An example (nearly by definition) doesn't impact the behavior of an implementation, so it doesn't really make sense.

In general, MUST and REQUIRED are reserved for functionality that is required in order to have interoperability.

Contrast this to e.g.

The value of "multipleOf" MUST be a number

While this still isn't proscribing a specific behavior for implementations, it has the effect of allowing implementations that see a non-number here to produce undefined results. I don't think it really makes sense to apply the same to an "example" keyword, where the majority of the time it's going to be ignored.

I don't really see the need to get into the business of figuring out when we should verify an example and who that would be reported to, implementations can figure that out depending on their needs.

I might also suggest an "examples" (plurl) that can provide an array or object of examples.

@geemus
Copy link
Collaborator

geemus commented Nov 4, 2016

@jdesrosiers I think those intentions make sense, I was thinking about the implied user guidance more than literal validation. As you said, it would be BAD to have invalid stuff here, but probably doesn't need to actually be a validation concern for implementors. That said, maybe it wouldn't hurt too much? I honestly don't have particularly strong feelings here, it just stuck out as being a bit odd without context (but you have now supplied that).

I would suggest pushing discussion of MUST/etc to #125 as it is pretty orthogonal to this issue now.

@awwright I can certainly see the value of it being a string or array, but in looking for other properties with this characteristic the only one I was seeing was type, which despite being this way is NOT pluralized. Perhaps it should be a change to the described usage without changing the keyword to be consistent?

@handrews
Copy link
Contributor Author

handrews commented Nov 4, 2016

@awwright For "examples" do you mean in addition to or instead of? I could go for instead of. In addition to seems like overkill, and we can't use the same keyword to mean both because (unlike type) the results could be ambiguous.

But I could definitely support just having "examples" and requiring an array. @geemus what do you think of that option? type is not pluralized because it's usually singular but can be an array (which I think came later).

@geemus
Copy link
Collaborator

geemus commented Nov 4, 2016

@handrews I would agree that having both keys seems like overkill. A single key that is either one value or array seems good, and I'm fine with either pluralization for the key.

@handrews
Copy link
Contributor Author

handrews commented Nov 4, 2016

OK now it's plural, and I adjusted the reference to "default" appropriately as well. I left the MAY wording in as that's being addressed in #125 and if it results in a change can be done in a follow-up pull request.

JSON UI Schema aside, most people seem to approve of the usage of default as a supplementary example, so I left that in. Again, it's a MAY feature, so @epoberezkin if you don't want to implement it in ajv you are not required to.

@epoberezkin
Copy link
Member

@handrews that's ok, these are not validation keywords anyway.

@handrews
Copy link
Contributor Author

handrews commented Nov 7, 2016

OK, I think all concerns have been resolved? We need three approvals to get it in (presumably in addition to @geemus as he requested it)
@awwright @Relequestual @epoberezkin @jdesrosiers any of you willing to make it official?

</t>
<t>
This keyword MAY be used in root schemas, and in any subschemas.
</t>
Copy link
Member

Choose a reason for hiding this comment

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

Does this paragraph serve any purpose? It's describing the default behavior for any keyword.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other meta-data keywords have that line as well, so I was trying to be consistent. I can take it out of all three places if you'd like- I think it should be in all or none.

@awwright
Copy link
Member

awwright commented Nov 7, 2016

It seems like this is a good feature to add, I just added one comment about a paragraph. Lemme know what you think.

@handrews
Copy link
Contributor Author

handrews commented Nov 8, 2016

Updated to remove all three instances of that MAY be used in root schema or subschemas line. It only appeared in the meta-data section so it seemed better to just drop it.

@geemus
Copy link
Collaborator

geemus commented Nov 9, 2016

@handrews LGTM 👍 :shipit:

This addresses issue json-schema-org#121.

Most of the wording is lifted from "default", which is similar and can serve
the same purpose.  This wording explicitly allows for using "default" as an
example, without requiring that implementations do so.

The "MAY be used in a root schema or any subschema" line for all of the
meta-data keywords seems to have been a holdover from an earlier approach
of always specifying where keywords could be used.  It is not done anywhere
else and raised a red flag in review of the original version of this
commit, so this version removes it from all meta-data keywords.
@awwright awwright merged commit 4650004 into json-schema-org:master Nov 21, 2016
@awwright awwright mentioned this pull request Nov 21, 2016
@handrews handrews deleted the example branch November 21, 2016 18:02
This keyword MAY be used in root schemas, and in any subschemas.
Implementations MAY use the value of "default", if present, as
an additional example. If "examples" is absent, "default"
MAY still be used in this manner.
</t>
Copy link
Member

Choose a reason for hiding this comment

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

RECOMMENDED that these values be valid against the associated schema.

I think it should be "MUST BE".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was discussed (for default and examples) in #125, and we decided not to change the approach.

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.

7 participants