Skip to content
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

extract propertyDependencies #1450

Closed
wants to merge 9 commits into from

Conversation

gregsdennis
Copy link
Member

Relates to #1444 and #1449

@gregsdennis gregsdennis requested review from Julian and a team September 27, 2023 08:23
@gregsdennis
Copy link
Member Author

gregsdennis commented Sep 27, 2023

There's also a change to the 10.2.2 text that would need to happen, but if #1451 goes through, then it'll be addressed anyway.

Three of these keywords work together to implement conditional application of a subschema based on the outcome of another subschema. The fourth is a shortcut for a specific conditional case.

The fourth that it's talking about is dependentSchemas, which would make propertyDependencies the fifth, so this text would need to change, and the proposal should include that.

But again, if #1451 goes through, this is moot.

@jdesrosiers
Copy link
Member

I just pushed an update filling in the missing sections as well as some refactoring. The following is a summary of the changes I made.

I'm concerned about the "proposal" terminology used through out the document. It's in the proposal stage now, but at some point it will be at a stage where it's expected that all implementations should be implementing it. It can't be promoted to stable until a large majority of actively maintained implementations support the feature. I think that prominently calling it a proposal will hinder that process. People will be less likely to implement something called a "proposal". We've seen how words like "draft" can effect people despite anything we say.

In place of using the word "proposal" the way it's being used, I've added a "Status" section to explain the status of the document. This needs to be filled and we need to agree on some of the details before that can be done.

I found that the "Overview" section ended up being quite large and that it ends up burying the primary content of the document that people are mostly interested in when viewing this document. My solution was to keep those sections as short as I could and link to appendices with more information.

I removed the "Examples" section because I thought the "Solution" section covered that sufficiently.

I made the "Limitations" section a subsection of the "Solution" section because I felt like that made sense as the limitations are a consequence of the solution.

Finally, I added a "Change Log" section.

Copy link
Member Author

@gregsdennis gregsdennis left a comment

Choose a reason for hiding this comment

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

I think the content is fine, but I disagree with reordering the sections.

I found that the "Overview" section ended up being quite large and that it ends up burying the primary content of the document that people are mostly interested in when viewing this document. My solution was to keep those sections as short as I could and link to appendices with more information.

I put a lot of thought into the ordering of the sections.

With this new ordering, to get the full context, the reader has to jump around the document.

As they were, they the reader gets the ideal experience: they get a story that tells them what the problem is, what the solution is, what its shortcomings are, and what alternatives were considered (i.e. why the particular solution was chosen).

Casual readers (i.e. most implementors and definitely most schema authors) won't care about the actual spec change. They'll trust us to get the wording just right so that it adequately describes the feature. The meat of the proposal is the what and the why, and that should go at the top. The spec change is bookkeeping; it can go at the bottom. (There's also a TOC, so they can go straight to the spec changes if they want.)

Regarding the length, we don't need a full history or extensive explanations. That should be documented in the linked issues. We're not looking to convince the reader that the change is needed. We've already decided that it's needed. We just need a quick summary of why.

I'm concerned about the "proposal" terminology used through out the document.

This document is precisely a proposal to change the specification. I understand your preference to not use that word, so if you want to use a different word, fine. But it doesn't change the fact that this and future documents like it are proposals for change.

I've added a "Status" section to explain the status of the document.

I removed the "Examples" section because I thought the "Solution" section covered that sufficiently.

I added a "Change Log" section.

I'm fine with these.

I made the "Limitations" section a subsection of the "Solution" section...

You did not. They're both still H3, which I think is fine.

those goals means that some trade-offs need to be made. {{alternatives}} lists
some alternatives that we considered.

## A Vocabulary for Applying Subschemas
Copy link
Member Author

Choose a reason for hiding this comment

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

The title "Target for Change" is not a template title intended to be replaced with the section. It is a section of this document that indicates the specifically where and in what document the proposed change should be applied. It needs to stay "Target for Change".

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I forgot to explain why I changed that. This was a combination of a few things. I thought it was unnecessary for the "Proposal" content to be empty. The "Target for Change" content makes perfect sense in that space and we don't need the extra section.

After moving that content, my next thought was that the content was fragile. It's referencing a section number and its title in the spec. Either of those could change and that reference would be wrong. I wanted to both reduce that fragility and make it more explicit that the change is to add this keyword to a specific vocabulary. So, I changed it to reference the vocabulary URI the keyword is being added to. In theory that should be permanent and never change. To be helpful, I also linked to the section in the spec that describes the vocabulary. I added named anchor to use for this reference specifically to make it less fragile.

However, later I was trying to replace the Proposal heading with something that doesn't use the word "proposal". I couldn't think of anything at the time, but thought it would be interesting mimic the hierarchy of the spec. (Interestingly, that's also what they did here.) Of course, this goes against the anti-fragile goal I had for the previous change, but I had no other ideas at the time. What I probably should have done was bring back the "Target for Change" I had removed from before and used that heading instead. However, if you're ok with the word "Extension", I think I like that better. Let me know which you prefer and I'll drop the spec-matched hierarchy. I wasn't happy with the fragility of that anyway.

Comment on lines 92 to 95
This document adds the `propertyDependencies` keyword to the
`https://json-schema.org/vocab/applicator` [applicator
vocabulary](../jsonschema-core.html#applicatorvocab).
Copy link
Member Author

@gregsdennis gregsdennis Oct 4, 2023

Choose a reason for hiding this comment

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

For reference, the text I had here was

This proposal will add the {{propertydependencies}} section contained herein as
a subsection of JSON Schema Core, section 10.2.2 "Keywords for Applying
Subschemas Conditionally."

I don't mind the soft reference to the vocabulary, but it needs to specify exactly what needs to be changed:

  • which document? - Core
  • where in the document? - Keywords for Applying Subschemas Conditionally
  • what exactly is changing? - we're adding a new section, defined below

Copy link
Member

Choose a reason for hiding this comment

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

I think this is another case of us having different ideas about what this document is for. I'm thinking that this document should be able to stand on its own as an extension to JSON Schema whether it gets accepted into the spec or not is a separate concern. It's use/implementation isn't dependent on being accepted to the spec, so I'd like to have some decoupling in that respect. It seems plenty to say its name and what vocabulary its part of and then when/if it's integrated into the spec, where exactly it ends up is the concern of the document it's being integrated into.

If you like the separate documents idea, I can see information about where exactly the change is expected to go to be in the "proposal" document, but honestly, I don't think it's a concern until we're actually at the stage of putting it into the spec.


Omitting this keyword has the same behavior as an empty object.

## [Appendix] Problems With Existing Patterns {#problems}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is part of the problem. It should be part of the problem statement, not an appendix.

Having the problem statement before the solution builds a story for the reader. It explains why the change is needed.

}
```

## [Appendix] Alternatives Considered {#alternatives}
Copy link
Member Author

@gregsdennis gregsdennis Oct 4, 2023

Choose a reason for hiding this comment

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

The alternatives need to be listed in the same area as the limitations (which need to stay with the solution). This should be moved back up.

Comment on lines 86 to 89
actually use it rather than fallback to `oneOf` because it's simple. Achieving
those goals means that some trade-offs need to be made. {{alternatives}} lists
some alternatives that we considered.
Copy link
Member Author

@gregsdennis gregsdennis Oct 4, 2023

Choose a reason for hiding this comment

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

I don't think these last couple of sentences are needed. I think trade-offs are implied by having a "limitations" section.

Suggested change
actually use it rather than fallback to `oneOf` because it's simple. Achieving
those goals means that some trade-offs need to be made. {{alternatives}} lists
some alternatives that we considered.
actually use it rather than fallback to `oneOf` because it's simple.

Copy link
Member

Choose a reason for hiding this comment

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

The purpose of those words is to say that none of the alternatives we considered satisfy our goals for the keyword. I think that's important to say. It also provides a link for readers who want to do a deep dive on what alternatives we considered. I think that's important too. Maybe those sentences could use some wordsmithing, but I think they're both saying important things.

@gregsdennis
Copy link
Member Author

I've merged the output extraction PR, which I think made changes to the HTML builds. This PR should overwrite those changes, but for now, they're in conflict.

@gregsdennis
Copy link
Member Author

cc: @jdesrosiers ☝️

@jdesrosiers
Copy link
Member

@gregsdennis, it seems we have different ideas about the purpose of documents like this one. I was thinking about these primarily as spec documents, while it seems you're looking at it primarily as a proposal. I'm specifying the feature and you're making the case for why people should implement it and why it should be in the spec. That's why I was a bit surprised to see all the things in the template that you expected. I can see why we might want to include them, but I also didn't want them to drown out what I thought of as the main purpose of the document, which was specifying the feature.

To get some inspiration, I looked at what TC39 does. Unfortunately, it looks like there is no consistent template that they use. All of their proposals are different. Some look a lot like your template, while others link out to other places where the feature is more formally specified. Others are either of those. My favorite is in two distinct parts. The "proposal" document is the sales pitch as you envisioned, but it links out to a formal spec as I envisioned. What do you think about using this kind of approach of having two documents, one for the "why" and another for the "what"?

@jdesrosiers
Copy link
Member

jdesrosiers commented Oct 6, 2023

With this new ordering, to get the full context, the reader has to jump around the document.

What I was going for was to give readers the tldr summary (because it is quite long and tedious) while maintaining the order you laid out and also providing extra information as an appendix for people who wanted a deeper dive. It's possible that I didn't get that balance quite right, but certainly I don't see it as a reordering.

I understand your preference to not use that word, so if you want to use a different word, fine.

I've been thinking about this. If you're ok with the idea of having two documents, I think it makes sense to call the "why" document "proposals" and the "what" document "extensions". I think "extension" is an accurate description and is less likely to be ignored by implementer than something called "proposal". I see the "proposal" document as something a little more internal like something that only lives as a markdown file in Github, while the "extension" spec would be published on the website alongside the stable spec. Of course the status of the extension spec would be clearly marked and the proposal would be linked to.

I made the "Limitations" section a subsection of the "Solution" section...

You did not. They're both still H3, which I think is fine.

Ha. I thought I did. I don't feel strongly about this one. Just trying it out.

@jdesrosiers jdesrosiers force-pushed the gregsdennis/extract-propertydependencies branch from fdf85c1 to 8c2a6ee Compare October 6, 2023 19:54
@jdesrosiers jdesrosiers force-pushed the gregsdennis/extract-propertydependencies branch from 8c2a6ee to 0b1a71a Compare October 6, 2023 20:18
@gregsdennis
Copy link
Member Author

gregsdennis commented Oct 8, 2023

This is more of an ADR-like document than a spec-like document in that we've discussed the feature somewhere else (likely multiple "somewhere else"s) and made a decision on the direction of the feature. Now that decision needs to be officially documented. Additionally, as part of documenting the decision and to enact it, a proposal to change the spec needs to be included. Therefore this document serves two purposes:

  • capture the decision (like an ADR: not extensive; just a summary)
  • define the language for the specification

We're not trying to convince people with this document. The decision was already made to augment the specification. We're not justifying it; we're documenting it. As part of that documentation, we need to know what that augmentation looks like.

Splitting this into multiple documents carries the same problems as reordering the sections (or having some of the reasoning in an appendix, or whatever you want to say you did). It forces the reader to navigate to multiple areas to get the full story.

I've been part of multiple companies that have independently developed documents like this to capture decisions around changing processes, company best practices, and other aspects of building software, and this is consistently the format. It answers all of the reader's questions in an easy-to-consume way:

  • What's the problem?
  • What's the solution?
  • Why did we choose this?
  • What alternatives were considered?
  • What are the technical details?

A less-technical reader (I expect most schema authors, but likely even some managers and product owners) can scan the first part and get an idea of what the change is (perhaps a dev is proposing that they use this experimental JSON Schema feature to solve a problem in their product). A more technical person (generally devs and implementors) can continue on to the technical bits. If you want more buy-in on the feature, then you need a wider audience. If you put the technical stuff at the front, you lose that first segment of readership.

One document: decision at the beginning, change proposal at the end.


Secondly, it should be called a proposal because that's what it is. It's separate from the specification, but it's not intended to remain separate; it's a proposal to change the specification.

We can encourage implementors to support these proposals. We can even put a section to that effect in the spec as a SHOULD.

But it is a proposal, and we shouldn't shy away from calling it that. Giving it another name isn't going to magically make implementors more likely to support it.

@jdesrosiers
Copy link
Member

I like the comparison to an ADR. That's how I understood it after your initial response, but that analogy explains the purpose better than I did. I completely agree that we should have an ADR-like proposal document, although I didn't realize that's what you intended the purpose of this proposal to be at the time I wrote it up. I was writing it up as a spec document that includes some ADR-like content because I didn't expect we had a plan for that sort of thing elsewhere. You were expecting the opposite.

Let's do both. I think there's a lot of benefit in the proposal and extension being separate documents. It would make a lot of sense for the proposal document to be a proposal for JSON Schema to adopt the extension spec into the main spec. The same could be done for a third-party extension being proposed to be include in the spec. Even if we decide not to adopt the proposal, it could still exist as an independent extension spec that people can implement if they choose.

I hear your concerns about using multiple documents making it harder to consume, but I think that depends on the audience. Different audiences care about different things. For example, implementers generally don't care about ADR details like what alternatives were considered. An implementer is just there to know what to build. With everything in one document and details of what to build at the bottom, the implementer needs to scroll past a bunch of stuff they don't care about in order to get to the information they need. So, for the implementer, having a separate document in more ideal.

But it is a proposal, and we shouldn't shy away from calling it that.

I strongly disagree. It has also been argued that our releases are "drafts" and we shouldn't shy away from calling them that. However, we've clearly seen the negative effect that choice has had. That's taught me that we need to be careful about what terms we use and consider how people might interpret them. I think "extension" is equally applicable and less likely to be interpreted in a way we didn't intend.

@jdesrosiers
Copy link
Member

I know we're considering this blocked for now. I'm not considering what I just pushed as the direction we've chosen. Take it as an example. We can still change direction.

I split it into two documents: an extension specification and a proposal to adopt the specification in JSON Schema. I updated the proposal document to closely match the original proposed template as requested. Other than extracting the specification part into it's own document, I made one small change. I switched the "Alternatives" and "Limitations" sections. I thought it flowed better to have "Limitations" first because the limitations were the driver of what alternatives were considered.

Comment on lines +13 to +15
This extension is in the early stages of development. Changes, including
breaking changes, are possible. At this stage, it's recommended that
implementations disable this keyword by default.
Copy link
Member Author

Choose a reason for hiding this comment

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

I expect at some point we'd move this to some sort of "statuses" document (or section in a larger process document). I don't think a definition of what the "EXPERIMENTAL" status generally needs to be included for these.

As we don't yet have such a document, I think having this is fine.

Copy link
Member

Choose a reason for hiding this comment

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

I forgot to explain what I was thinking here. I'm not sure it makes sense. I was just trying it out.

I expect that the concept of a proposal being a proposal to adopt an extension wouldn't be limited to our extensions. Some third-party may define an extension spec in whatever form they choose with their own ideas about the status of the document. So, the extension spec's status is a different concept than the proposal process status. I thought, "what would I put here if this was just a standalone third-party spec and the proposal process wasn't involved?"

While I might use a status like "EXPERIMENTAL" while the spec is third-party, I'd probably switch to something like "Proposed Standard" when it enters the proposal process. So, what I did doesn't quite makes sense. It's kind of half way in two places.

Copy link
Member Author

Choose a reason for hiding this comment

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

All that is whatever. It needs to be defined, and I'm just saying a document like this only needs to point to wherever that ends up being defined. It doesn't need to copy any text. It should just say, "This is my status. For more info about statuses, click here."

Comment on lines +13 to +17
TODO: We should have a short standard blurb outlining the stages involved in a
feature making its way to stable status.

TODO: Link to a document that describes the proposal => stable process in
detail.
Copy link
Member Author

Choose a reason for hiding this comment

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

Similar here. I think just a link to the statuses (we should decide on stages/statuses at some point) document would suffice.

Copy link
Member

Choose a reason for hiding this comment

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

I think it depends on how public-facing we expect this document to be. If the audience is mainly internal, then a link would be sufficient. If it's intended to be more public, I think it would be beneficial to have a short canned blurb about what the status means so causal readers can get the gist of what the status means without having to follow the link to a detailed document about our process.

Copy link
Member

Choose a reason for hiding this comment

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

I think it depends on how public-facing we expect this document to be.

Disagree here. We should do what the simplest is for us in this case. My feeling is that these documents are going to be pretty meaningless without a reasoable understanding of the process and where such a document currently sits within that.


## Status

**Current Status**: PROPOSAL
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd expect this should match the extension doc.

Copy link
Member

Choose a reason for hiding this comment

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

implementations disable this keyword by default.

This extension has been
[proposed](https://github.com/json-schema-org/json-schema-spec/blob/main/proposals/propertyDependencies.md)
Copy link
Member Author

Choose a reason for hiding this comment

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

Here you're linking to the file in GH...


<!-- TODO: Update to where this actually ends up getting hosted. This is a guess
for now. -->
<https://json-schema.org/specification/extensions/propertyDependencies.html>
Copy link
Member Author

Choose a reason for hiding this comment

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

But here you're linking to a published doc. Seems like publishing this doc would help build a case for the feature's inclusion.

Copy link
Member

Choose a reason for hiding this comment

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

We were talking about this as an ADR type of document. Usually that kind of thing has an internal audience, so I was expecting that this would probably just be on GitHub. I'm not opposed to thinking about putting it on the website for more exposure, but I wouldn't put it up in the form of a spec document like our main specs or extension specs. What I had in mind, was that part of the proposal making it to Stage-1 (or whatever we end up calling it) was adding a UJS entry describing the keyword. That would expose users to proposed keywords that they might want to use and we can link to the proposal and the spec for people who want to dig deeper.

Copy link
Member Author

@gregsdennis gregsdennis Oct 17, 2023

Choose a reason for hiding this comment

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

Publishing it leans into my previous point about a dev needing to convince their mgr/superior to use an experimental feature. I think this content is useful for consumers of JSON Schema. Another reason I prefer the single doc approach.

Your concern before was that a dev wouldn't care about the "fluff" but would want to know about the stuff at the bottom and they shouldn't have to scroll all the way down there:

the implementer needs to scroll past a bunch of stuff they don't care about in order to get to the information they need

They'll get a TOC that they can use to go straight to what they need.

Copy link
Member

Choose a reason for hiding this comment

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

Publishing it leans into my previous point about a dev needing to convince their mgr/superior to use an experimental feature.

Good point. That's a reason to consider having the audience be more than just internal. However, I'm not sure that necessarily means it should be on the website. The document is still public on GitHub.

I think this content is useful for consumers of JSON Schema.

I think having something specifically for consumers and authors in UJS would be more useful because it's specifically focused for that audience.

Another reason I prefer the single doc approach.

I'm not seeing the connection here. I think the separate documents approach makes sense in this case. The manager cares about what the trade-offs are. They can get that just from the proposal document. The consumer/author cares about what the feature does and how to use it. They can get that from just the UJS documentation. The implementer cares about what they need to build. They can get that from just the spec. Of course there's going to be some overlap and people wearing multiple hats, but I think the separation of concerns by audience makes a lot of sense.

They'll get a TOC that they can use to go straight to what they need.

I take your point, but that doesn't address the other concerns I brought up. An extension could have a life outside of the proposal process. It might start off as an extension spec that was later proposed to be added to the spec. It might have been a proposal that was rejected, but still wants to live on as a third-party extension. Decoupling the spec from the proposal process allows for these cases.

@gregsdennis
Copy link
Member Author

I'm going to close this and try again. Some of the comments here were made before we created our process and are a bit outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants