-
-
Notifications
You must be signed in to change notification settings - Fork 282
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
Output Formatting #679
Output Formatting #679
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
I added some tiny comments about clarity, or about wording so that it is a bit more consistent with other parts of the spec. But they are just tiny tweaks, not major changes.
jsonschema-core.xml
Outdated
structure that matches the exact structure of the schema. | ||
</t> | ||
</list> | ||
Implementations SHOULD specify which levels of validation they support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to implement all levels up to and including the most complex/verbose one? Or could an implementation support only the Verbose option? We should be clear about that in this section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that an "up-to" requirement is necessary. Implementations should be free to support any of the formats independently. Maybe "levels" is wrong (I don't want to over-use "format," though)?
Personally, I found it easier to simply do the verbose format then mutate the output to get the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, updated. Please let me know if that works for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I originally described it an updated version would say that an implementation:
MUST provide flag
SHOULD provide basic
RECOMMENDED to provide detailed and
OPTIONALLY provide verbose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the verbose format is likely to be easier to implement I would prefer to allow any subset of formats be implemented. @Anthropic do you feel strongly about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super thanks for all your hard work on this, and sorry it's taken such an age for me to get to review it.
I think one or two of my comments are asking if changes are required, while the other are questions or comments to make sure I understand this right.
standardized-output-composition.md
Outdated
|
||
- `valid` - a boolean value indicating the overall validation success or failure | ||
- `errors` - the collection of errors or annotations produced by a failed validation | ||
- `annotations` - the collection of errors or annotations produced by a successful validation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, why would errors
include annotations (and the reverse)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the verbose output would do that. The primary cases to consider are the applicator keywords (especially not
). For example, an allOf
could contain 1 failed subschema, yielding an overall failure, but the verbose output would list all of the results. This would need to be inside errors
since it failed.
I agree it's a little counterintuitive, but I think it works better for automated processing of all the results are together rather than having both errors
and annotations
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so my follow up question here is:
If a subschema of not
fails, obviously the not
means the parent schema asserts valid is true. My question is then what happens to the annotations in the subschema? They only trickle up for schemas for which validation is a positive assertion, no? /ccing @handrews on this one =]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, you only return annotations if the assertion result is valid. Otherwise they are dropped from the final result set (although showing what they were before they were dropped in the more complex output format is fine and useful.
Some of this has changed in the XML version based on @handrews's comments. It's definitely worth reading that over this file. I can probably go ahead and delete it. |
jsonschema-core.xml
Outdated
<t> | ||
Because this output structure can be quite large, a smaller example is given | ||
here for brevity. The full output structure of the example above can be found | ||
[here](standardized-output-verbose.json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@handrews how do I do external links in this XML file? Probably not with Markdown as I have here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gregsdennis I think you want <eref>
. The full XML RFC reference is https://tools.ietf.org/html/rfc7749 (yes, that RFC is marked obsolete, but it's the right one- the xml2rfc tool still doesn't support the new RFC for xml2rfc, ironically).
jsonschema-core.xml
Outdated
<section title="Output validation schema"> | ||
<t> | ||
For convenience, a JSON Schema has been provided to validate output generated | ||
by implementations. It can be found [here](schema-output.json). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another external link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy with the changes. Just one outstanding question regarding the JSON Pointer for paths is unresolveable when including by-reference applicators. At least, by means of standard resolvers.
@handrews @Relequestual I've updated per all of your comments (thank you). I think the only thing that remains is to ensure that the two new files are deployed with the HTML and TXT files. I looked at the makefile, but couldn't understand it enough to be confident in editing it. I figure it'd be the same as how the meta-schemas are deployed, but I didn't see where that was done. |
Thanks @gregsdennis I will get to this in the next day or two. Really :-P The meta-schema (and XML) deployment process is manual and somewhat absurd. I don't even know what the makefile does other than generate the .html and .txt. I think we've gotten a PR or two around it but it was always someone wanting to do something else with them instead / in addition. Given that they get deployed roughly twice a year, by me, it's incredibly low on my priority list to change. |
This is looking great! I think all of my concerns have been addressed. |
@Anthropic I've updated the language around format support requirements. Please have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, this is the first day in nearly a week that I've felt well enough to think clearly.
jsonschema-core.xml
Outdated
to support the "detailed" format, it is not also required to support the "basic" | ||
or "verbose" formats, and vice versa. Implementations SHOULD specify in their | ||
documentation which formats of validation they support. | ||
Implementations MUST provide support for the "flag" format and SHOULD provide the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early on, we were going to avoid MUST requirements, on the grounds that implementations can take many forms, including those with extreme memory constraints, or streaming implementations, or other things we can't anticipate where there may be a pretty compelling reason for just a boolean rather than an object. I'm a little hesitant to use MUST here, although I understand the desire for interoperability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could support an approach where we say that implementations SHOULD support one or more of these formats, and if they do, then they MUST support flag, SHOULD support basic, etc.
That leaves some wiggle room for implementations that may be working under constraints that we cannot anticipate.
jsonschema-core.xml
Outdated
or "verbose" formats, and vice versa. Implementations SHOULD specify in their | ||
documentation which formats of validation they support. | ||
Implementations MUST provide support for the "flag" format and SHOULD provide the | ||
"basic" and "detailed" formats. The "verbose" format is OPTIONAL. Implementations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use
"flag" format, SHOULD provide the "basic" and "detailed" formats, and MAY provide the "verbose" format.
because we use MAY a lot in these documents, but rarely if ever use OPTIONAL. I don't think there's any reason, it just seems to be the convention that has developed organically.
I don't feel strongly about this, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I was sure there was a way to make it smoother.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all of the hard work @gregsdennis and everyone who participated in the discussion. This is now ready to merge!
Here is my first go at the output formatting. Happy to update as necessary. It was quite difficult coming up with examples that showed the flexibility of the output while also being concise.
The .md file should be removed before merging. It's just a composition file.This PR resolves #643 and relates to #396.