-
-
Notifications
You must be signed in to change notification settings - Fork 318
Cleanup and fixes for structuring #162
Cleanup and fixes for structuring #162
Conversation
Here's the link to deploy preview for the page that changed, https://deploy-preview-162--lucid-bardeen-10edfb.netlify.app/structuring.html |
I pushed up some changes mostly incorporating feedback from @about-code, but also including a few things I thought to add or rework while making those changes. |
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've made a few comments on your suggested re-write. Largly I think it's great and will be super helpful.
I've mostly made minor nit picks, but which I feel will help the overall readability.
In the bundling process, we often reference a "JSON Schema resource" or just a "Schema resource", as per https://json-schema.org/draft/2020-12/json-schema-core.html#rfc.section.4.3.5
A JSON Schema resource is a schema which is canonically identified by an absolute URI.
I think it would be helpful to do a parse of this re-write to check for where the term should be used, and I think it's worth defining it here again quoting from the spec.
Otherwise, this is really shaping up great! Looking forward to reviewing more!
source/structuring.rst
Outdated
necessarily network accessible. They are just identifiers. | ||
Implementations generally provide a way for you to load schemas into | ||
an internal schema database and they retrieve schemas from there when | ||
they are referenced. |
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.
they are referenced. | |
references are resolved. |
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 think the point is to get rid of the "they" and I'm on board with that. However, I don't think this suggestion quite makes sense. I'll see if I can figure out a better way to express this whole paragraph.
@Relequestual The next revision is ready for review. Most of the small cleanup stuff is in one commit and a couple of bigger changes are in separate commits. I ended up moving some of the sections around to better present the differences between schema identification and the base URI. I think it's better this way. I moved the sections and made changes to those sections in separate commits to hopefully make it easier to review, but I suggest having a skim from top to bottom to see how the new flow works. |
I didn't feel like this page presents the concepts very well. I ended up rewriting the entire thing with the exception of the introduction and the "Recursion" section. I think this revision does a better job at presenting the concepts and is definitely more complete. I wanted to include an example of how to use `$ref` and `definitions` to make schemas more readable, but I'm having trouble making progress on that (I'm foundering trying to come up with a real-ish example), so I'm leaving that out for now. Co-authored-by: about-code <6525873+about-code@users.noreply.github.com> Co-authored-by: Ben Hutton <relequestual@gmail.com>
I didn't feel like this page presents the concepts very well. I ended up rewriting the entire thing with the exception of the introduction and the "Recursion" section. The diff is going to be completely useless. I suggest reviewing the deploy preview. I think this revision does a better job at presenting the concepts and is definitely more complete. Let me know if anything is unclear or missing. Some parts border on being opinionated, so please push back if you disagree with the approach I present.
I wanted to include an example of how to use
$ref
anddefinitions
to make schemas more readable, but I'm having trouble making progress on that (I'm foundering trying to come up with a real-ish example), so I'm leaving that out for now. Might add something in later.Fixes #104
Fixes #160