json-schema-org / json-schema-spec

The JSON Schema specification
http://json-schema.org/
Other
3.82k stars 266 forks source link

Output nodes should be per subschema not per keyword #1249

Closed gregsdennis closed 2 years ago

gregsdennis commented 2 years ago

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:

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 commented 2 years ago

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

jdesrosiers commented 2 years ago

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).
karenetheridge commented 2 years ago

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.

gregsdennis commented 2 years ago

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

jdesrosiers commented 2 years ago

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.

gregsdennis commented 2 years ago

@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!

jdesrosiers commented 2 years ago

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.

gregsdennis commented 2 years ago

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.

handrews commented 2 years ago

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

gregsdennis commented 2 years ago

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.

jdesrosiers commented 2 years ago

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.