Skip to content

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

Merged
merged 2 commits into from
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.

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.
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.

@@ -799,8 +801,9 @@
<t>
When "if" is absent, or the instance successfully
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

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

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.

@@ -780,8 +781,9 @@
<t>
When "if" is absent, or the instance fails to
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

Choose a reason for hiding this comment

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

See comment below

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

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.

There is no default behavior for any of these keywords
when they are not present. In particular, they MUST NOT
be treated as if present with an empty schema, and when
"if" is not present, both "then" and "else" MUST be
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@handrews handrews merged commit 4d4d0bd into json-schema-org:master Mar 14, 2018
@handrews handrews deleted the if-fixup branch March 14, 2018 21:04
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.

One comment. Otherwise, yes, thanks, this meakes things clearer!

This validation outcome of this keyword's subschema
has no direct effect on the overall validation
result. Rather, it controls which of the "then"
or "else" keywords are evaluated.
Copy link
Member

Choose a reason for hiding this comment

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

/evaluated/are applicable/ ? Thinking we should use applicability termonology where possible.

@gregsdennis gregsdennis added clarification Items that need to be clarified in the specification and removed Type: Maintenance labels Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Items that need to be clarified in the specification Priority: High validation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants