Fix missing "if present" for "else" #554

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

Jump to bottom

Merged

handrews merged 2 commits into json-schema-org:master from handrews:if-fixup

Mar 14, 2018

Merged

Fix missing "if present" for "else" #554

handrews merged 2 commits into json-schema-org:master from handrews:if-fixup

Mar 14, 2018

Conversation

@handrews

Copy link

Contributor

@handrews handrews commented Mar 2, 2018

Also other minor wording clarifications for if/then/else such as
standardizing the "regardless of the validation outcome against
the subschema" language. That language is important as it
clarifies that subschema validation can fail (causing annotations
to be dropped) without causing the containing schema to fail.

@handrews


 Fix missing "if present" for "else"

d924055

Also other minor wording clarifications for if/then/else such as
standardizing the "regardless of the validation outcome against
the subschema" language. That language is important as it
clarifies that subschema validation can fail (causing annotations
to be dropped) without causing the containing schema to fail.

@handrews handrews added Type: Maintenance validation labels

Mar 2, 2018

@handrews handrews mentioned this pull request

Mar 3, 2018

handrews-*-01 bugfix for core & validation #555

Merged

@handrews handrews added the Priority: High label

Mar 6, 2018

Relequestual

Relequestual requested changes

Mar 8, 2018

View reviewed changes

Copy link

Member

@Relequestual Relequestual left a comment

Choose a reason for hiding this comment

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

I'm not sure this helps anything, and more fixing may be required. See comments.

jsonschema-validation.xml Outdated

validates against its subschema, validation against

this keyword always succeeds. Implementations

SHOULD avoid attempting to validate against

this keyword always succeeds, regardless of the

Copy link

Member

@Relequestual Relequestual Mar 8, 2018

Choose a reason for hiding this comment

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

I know this isn't your change, but I find the phrasing...
"When "if" is absent, or the instance successfully validates against its subschema, validation against this keyword always succeeds..."

This reads to me as, when the subschema of if successfully validates, else always succeeds [validation], regardless of the actual validation result of the instance against it's subschema.

That can't be the intent...

Copy link

Contributor Author

@handrews handrews Mar 8, 2018

Choose a reason for hiding this comment

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

Great feedback, I'll play around with this some more.
The main point of bug-fixing the validation spec is to clarify if/then/else so please highlight everything that's awkward.

jsonschema-validation.xml Outdated

validate against its subschema, validation against

this keyword always succeeds. Implementations

SHOULD avoid attempting to validate against

this keyword always succeeds, regardless of the

Copy link

Member

@Relequestual Relequestual Mar 8, 2018

Choose a reason for hiding this comment

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

See comment below

@handrews


 More thorough rewrite of "if"/"then"/"else"

0bc8cde

Further feedback indicates the language was still confusing.
Rewrote it to be more explicit about how the keywords interact
with each other and with both validation and annotation
collection.
In particular, an "if" on its own collects annotations but
never affects validation, annotations are only collected from
"them" or "else" if their use is indicated by the validation
outcome of an adjacent "if", and there is no default behavior
for any of the keywords.
Technically, "then" could be assigned an empty schema default
without causing problems, but that felt weirdly unbalanced.
If "if" has a default of true/empty schema, then "then" on
its own would be active, which is counter-intuitive. If "else"
had a default it would need to be false rather than true, and
saying it has no default seemed simpler, and matches how "not"
is handled. "not" is the only other keyword that would otherwise
need a false schema as a default value.

@handrews

Copy link

Contributor Author

handrews commented Mar 13, 2018

Rewrote "if" etc. to be more explicit about how the keywords
interact with each other and with both validation and annotation
collection.

In particular, an "if" on its own collects annotations but
never affects validation, annotations are only collected from
"them" or "else" if their use is indicated by the validation
outcome of an adjacent "if", and there is no default behavior
for any of the keywords.

Technically, "then" could be assigned an empty schema default
without causing problems, but that felt weirdly unbalanced.

If "if" has a default of true/empty schema, then "then" on
its own would be active, which is counter-intuitive. If "else"
had a default it would need to be false rather than true, and
saying it has no default seemed simpler, and matches how "not"
is handled. "not" is the only other keyword that would otherwise
need a false schema as a default value.

Note that the meta-schema always puts in a default, but this was
already true (and incorrect) for "not", so I'm not worrying about it
now. We can fix it in draft-08 if necessary.