json-schema-spec icon indicating copy to clipboard operation
json-schema-spec copied to clipboard

Output nodes should be per subschema not per keyword

Open gregsdennis opened this issue 2 years ago • 11 comments

This is quite a change, so I'd be happy to walk anyone through it.

Summary

Following the idea from this thread and this thread, this PR changes the concept of the output unit from being based on individual output from keywords to being based on output from schemas. The benefits of this are in the thread, so I won't rehash them here. The primary difference is that errors and annotations are included in the output unit as properties (objects with keyword names for keys) rather than as nested units.

It also does a few other things:

  • removes the detailed format
  • renames the verbose format hierarchical (since there's only one now)
  • adds clarification in some of the language
  • adds passing examples to show how annotations are included

Doing this made the formats considerably more information-dense, which meant that the verbose examples now reasonably fit in the spec rather than needing to be defined in an external file.

All of this is pretty related, otherwise I would have broken it into multiple PRs.

gregsdennis avatar Jun 27 '22 23:06 gregsdennis

I have implemented this in the schema/experiment-new-output-format branch of json-everything

gregsdennis avatar Jun 30 '22 20:06 gregsdennis

The draft-next branch has been merged and is now closed. The merge target for this PR has been changed to main. Here are the recommended steps to get your branch reabsed properly.

  1. Make sure your remote for the json-schema-org/json-schema-spec repo is up-to-date. (Example: git fetch upstream).
  2. Rebase your commits onto main. (Example: git rebase --onto upstream/main abcd123~1 (replace abcd123 with the commit hash of the first commit in your PR)).
  3. Force push the rebased branch to your fork. (Example: git push --force origin my-branch).

jdesrosiers avatar Jul 08 '22 15:07 jdesrosiers

I am strongly against parts of this PR but I am unable to spend much time going through it again in detail until the end of the week.

karenetheridge avatar Jul 11 '22 16:07 karenetheridge

@karenetheridge I welcome your feedback, but it's been over two weeks since you posted.

gregsdennis avatar Jul 27 '22 22:07 gregsdennis

I'm still not entirely sure how I feel about the approach. I think it will work fine. I think my only concern is that we would be making a drastic change without really knowing how it's going to work out. Not that what we have now is well proven either, I just don't want to be implementing a completely new output format every release. I don't know that there's really a solution to that without a crystal ball, but I wanted to mention it anyway.

jdesrosiers avatar Aug 04 '22 21:08 jdesrosiers

@jdesrosiers I understand and share that concern. However, as I mentioned in my blog post, the current design was not met with much acceptance and there were a lot of questions.

Secondly, I think this aligns better with the discussion we had around how annotations are passed on / blocked.

It really needs to change. I really think that we're a lot closer to a final form.

Also, having implemented both formats, I can tell you that this is so much simpler!

gregsdennis avatar Aug 04 '22 21:08 gregsdennis

I completely agree that it needs an update. Doing nothing is not an option. My main concern is maintaining a bunch of different versions of the output format as we iterate and experiment to figure out what's going to work best. I'd feel a lot better about this if it were it's own spec and we actually treated it like a draft where each draft replaces the previous and has no backwards/forwards compatibility guarantees until things stabilize. That way I always only have to maintain the latest version.

jdesrosiers avatar Aug 04 '22 23:08 jdesrosiers

I agree with what you're saying. For now, I think we can still make improvements in-place and then extract the output separately later.

gregsdennis avatar Aug 07 '22 21:08 gregsdennis

@jdesrosiers given that we quite likely won't even put out the next revision under the IETF process, I don't think we should worry too much about what is in which document at the moment.

handrews avatar Aug 07 '22 23:08 handrews

Let's continue the output-in-a-new-spec discussion in this thread. For now, it's all in Core. We'll open a new PR for further developments. I think this one is complete.

gregsdennis avatar Aug 08 '22 07:08 gregsdennis

given that we quite likely won't even put out the next revision under the IETF process, I don't think we should worry too much about what is in which document at the moment.

I don't think what process we are using makes a difference, unless your point is that it doesn't make sense to split things out until we know what process we're going to use moving forward. That makes sense, but I wasn't suggesting it be split out now, just that it's an important consideration to keep in mind. If we're going to make such a big change we need to consider the effect on implementors and that very much includes discussion of what the life-cycle of this feature will be.

I think it's a relevant an very important to thing to be thinking about now, but it's not a blocker for this PR, just an important thing to call out and follow up on later.

jdesrosiers avatar Aug 08 '22 18:08 jdesrosiers