-
-
Notifications
You must be signed in to change notification settings - Fork 266
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
Rename post-split "dependencies" to "schemaDependencies" #591
Comments
We had |
I don't follow. This is about a single keyword being split into two keywords, and the best way to name them while avoiding breaking existing implementations. What does that have to do with But the point here is not that |
You figured it out, I'm questioning if we need keywords to all be two words just for the sake of being the same number of words. I don't see why. Now if it's a change in behavior, then I see that. Do you have a simple example that demonstrates the change, to make sure I'm understanding it right? |
@awwright yeah, that's a reasonable question. The fact that it goes from one word to two is coincidental. draft-07: {
"dependencies": {
"foo": ["bar", "baz"],
"biz": {
"maxProperties": 10
}
}
} We decided in #528 (comment) to split {
"requiredDependencies": {
"foo": ["bar", "baz"]
},
"dependencies": {
"biz": {
"maxProperties": 10
}
}
} The problem here is that now These are things that are not compelling on their own, but they add up and slow adoption. So what I would like for draft-08 to look like is: {
"requiredDependencies": {
"foo": ["bar", "baz"]
},
"schemaDependencies": {
"biz": {
"maxProperties": 10
}
}
} There is no chance of mistaking either of those keywords for the old dual-function So, to summarize:
|
Since I can't get anyone to comment further on this despite reaching out in several forums, I'm moving to a PR. |
I missed emails on this sorry - will comment on it tonight.
|
@handrews {
"dependencies": {
"foo": {"required": ["bar", "baz"]}
}
} To me, this change really looks like a behavioral change in
How is it not the same? It looks the same to me, the only change is removing a syntactic shortcut.
Why does it need renaming for this though? There's nothing stopping implementations from continuing support for pre-draft-08 syntax; it's just a simple type switch (i.e. if the value is an array, either warn / error or provide deprecated support). I agree that UX should be a priority, however I think that the eventual UX of the final standard is more important than the UX of migrating from one draft to another. As far as moving between drafts goes, making
Could you expand on this a bit? I really don't see how this is complicated from an implementation perspective. The backwards-compatibility logic is easy - if |
We already lost that argument a long time ago, no point in revisiting.
In everything I've ever implemented (an assortment of bits and pieces, most not public),
I strongly dispute this. In my experience,
It's complicated compared to other keywords. The original Also, implementing Your objections seem to boil down to not liking change, but the whole point of being in pre-release drafts is to facilitate change. I try to avoid it as much as possible (goodness knows there are other things I'd personally like to tweak), but when we have decided to make a change (as we have here), I want to make the change clear and obvious, and facilitate migrations as much as possible. What I'm looking for here is an affirmative argument for keeping it as |
Fair call :-).
Any chance you could expand on this? The logic is relatively straightforward, so I'm not sure why it should result in a messy implementation. Is there something obvious I'm missing here? The ugliest bits of implementation I've had to do with have mostly been:
Do you know what aspect of it has been confusing? It seems intuitive to me (i.e. if $property, then apply $schema or if $property then require $otherProperties), but perhaps I'm missing something?
It's complicated compared to some keywords, sure - but way less complicated than others. What is complicated about the following?
I agree completely. That is what removing the array type addressed - it's now a simple keyword that applies a schema, or not, based on whether a property is present.
Perhaps I haven't been clear enough - I will try to rephrase. I'm not objecting to change, so much as objecting to change without a good reason. I haven't seen a compelling enough reason to do so here.
Agreed.
I would like to address this specifically. Things breaking between drafts should be expected - it's a draft standard under development. Schema authors migrating from an earlier draft to a later one should expect to make changes to their schema. Implementation maintainers should expect their implementation to require new logic. Compatibility is important. However compatibility, where implementations desire it (and according to the spec they should) can be easily maintained without a name-change in this case. Going forward, attempting to use an array definition here will simply result in a type error. It's not a silent failure, it's a screamingly obvious your-schema-is-invalid error. If the change were behavioral in nature (e.g. if we kept the array form, but it now meant something else) then the potential for a silent change in the validation outcome would make the rename far more compelling - but as it is, keeping the name as-is does not cause any silent issues for users at all. |
This is a fair point, and I am certainly open to other names. That are not All the rest of this appears to be personal preference, based on differing notions of what constitutes good UX. And I can't help but feel that if I'd just done the names differently in the first PR that this would never have even come up, and we're stuck in this argument because I slipped up there. I am not going to further discuss the questions of what is messy or not, because it is subjective and we clearly have different views.
The behavior has changed. It no longer accepts a list of strings. If you do not believe that that constitutes a change, then I don't see any productive direction for this conversation. All of this is predicated on the fact that
No, it cannot. The string array behavior is gone, and an implementation that allows it would be in violation of the spec. However, if the split keywords are two new keywords, then
That is what I am doing- getting the new keywords out of the old, so that implementations can do whatever they want with the old.
All of these things are on a case-by-case basis, and validation and core keywords have barely changed at all in the last three specs. There is no requirement to do anything. I think it makes sense in this case.
The status quo is already changed, this makes no sense at all to me.
We have different notions of what constitutes good UX here. |
@erayd I don't know what to do with this. It's basically down to a difference of opinion. I look at this as fixing a bug in the originally agree-upon change, you don't. I honestly thought this would be totally uncontroversial, but I guess nothing ever is. Everything on this project is a giant argument, but at the end of the day I have to write the PRs and publish something. Do I decide in my favor because I'm writing the PRs? Do I avoid deciding in my favor because I write the PRs? Which gives everyone a veto when the differences aren't big enough to be obvious? I have no idea anymore. |
I think this may be the fundamentally the cause of us disagreeing on this.
I believe it constitutes a syntactical change, not a behavioral one. Whereas you consider a syntax change to be a kind of behavioral change.
As far as I'm concerned, if you feel that an executive decision is required to move things along, feel free to make that decision in your favour, and overrule my input. You carry much more of the load of this project than I do or ever have, and seem likely to continue to do so in the future. I may not like your decision, or agree with it, but I don't want to stall progress simply because I disagree with you :-). I've made my opinion known, but it is an opinion, and I am only one voice. You have proven to me on many occasions that you listen, and have good reasons for your decisions. As such, I'm quite happy to let you decide what the path forward should be. |
Thanks, @erayd. I'm poking one or two other people for a sanity check, as I'm always suspicious of myself when I want to overrule someone without a compelling technical reason. If I can't get that then I'll move ahead as-is. Regarding syntax vs behavior: I think entirely in terms of assertion/annotation/applicator classifications. |
That sounds like a good plan 👍
I think I get what you mean here - am I correct in my understanding that you're saying that it's less about behavior, and more that this change has fundamentally changed what class of keyword this is? And that because the keyword class has changed, that is inherently a major change, even though the cause of it was syntactical? |
Yes, exactly. To me, class == behavior. You're looking at it as a generalized "if property X is present, then take action Y". From that perspective, it's the same, because you don't care that much about what Y is. I think. I care far more about the nature of Y. "apply this subschema to the current location" and "assert that this property is present" are completely different things to me. The fact that there's an "if property X is present" involved in both is irrelevant. Or perhaps not irrelevant, but just coincidental. |
Yep, that's accurate - I'm seeing it more from the perspective of "what logical steps are required to implement this", and I wasn't thinking about the class distinction at all. Are you able to expand a bit more on why you feel the class distinction is so important (feel free to decline, or do so via email, if you think that's off-topic for this thread)? In a more general sense I mean, not so much specifically about this keyword - more around why it matters so much [to this project], when it's fundamentally an abstract concept. I think this is something that's fuelled many of the other debates on this project, so it would be useful to better understand your perspective on it. |
What about |
@erayd I've been trying to figure out a way to get that point across for a while, with little success. It's a diversion, but I might as well give it a shot here- if necessary I can delete it and file it elsewhere, but it's not like a ton of people have been commenting on this one :-) It has to do with transitioning JSON Schema from a primarily validation-oriented tool (with other things kind of as afterthoughts) to a modular and extensible system, with consistent keyword mechanisms and behaviors. In particular, generative use cases (UI generation, code generation, and documentation generation) are very well-established in the wild. More so than hyper-schema, really. Hyper-schema's primary use over the last several years has been for documentation, rather than runtime link generation. For most of these cases, what was needed to improve support for them was not more validation keywords, but other sorts of information (e.g. "this The appropriate thing to do seems to be to acknowledge that those are important use cases for JSON Schema as a system, and make them first-class citizens alongside validation. But while validation assertions are well-established, annotations have always been very underspecified. This has particularly been a problem with The best approach was to be more clear about what information JSON Schema implementations are expected to present to applications, and be clear that application should define how they make use of that information. See PR #610 and issues #530 and #396 for more details on this. The other part of this is applicators (a term that I don't really like, but haven't come up with anything better- and no, they don't all have to start with "a" although that does amuse me). Way back in draft-04, there was this awkwardly worded section about calculating children schemas, and a really complicated description of object validation. Array validation was nearly as complicated. @awwright cleaned that up tremendously in draft-05, but it got me thinking about why those keywords were treated like that in draft-04. Fast-forward a ways, and I'm talking with a maintainer of a UI generation library, who points out that while he often needs Finally, the whole I don't know if that helps at all- I figured I'd try narrating how and why I got here rather than trying to lay out the theory. In general, it's helpful for both answering questions of "where does this go? Is this within the current scope or a new thing? etc." and for spotting design smells like |
@erayd it's funny you should mention
I had preferred |
Assuming that we are proceeding with moving away from Regarding your long reply on classes - that's really helpful. I'd like to re-read some of the other discussions in that light, so will probably take a couple of days to digest it all, but it's giving me a much better handle on where you're coming from. Initial tentative first impressions are that (shockingly!) I may agree with you ;-). Will respond to that in a bit more detail once I've finished going over stuff. |
OK, I'm going to sleep on it but I like the I'll note that I even commented in #528 that we could tweak the names later, as I was not all that confident of them, but wanted to move forward for a variety of reasons that aren't relevant anymore. |
Having slept on it, I like |
Having read all relevant comments, I'm in favour of splitting into two new keywords, prefixed with |
This got merged back in June but I forgot about the issue. Closing now. |
Previously, we split the old
dependencies
keyword in two:requiredDependencies
is an assertion in the validation vocabulary, and behaves the same as the old string array form ofdependencies
dependencies
is an applicator in the core spec's applicator vocabulary, and only supports the old schema formThis makes
dependencies
incompatible from draft-07 to draft-08: anything using the string form is now incorrect and will not work. This is also confusing for schema authors migrating to draft-08: there is a non-obvious change and they need to change how they think about this keyword.Instead, let's do a true split, and make the schema case
schemaDependencies
. This does two things:requiredDependencies
dependencies
with its pre-draft-08 semantics as an extension keywordThe second point is particularly compelling. It also gives implementors the ability to continue to support the existing
dependencies
behavior with draft-08 (presumably as an opt-in, or even better with a custom meta-schema using the new vocabulary support allowing schema authors to declaredependencies
as an extension- we could even offer a "draft-08-transitional" meta-schema that provides this).I've been worried about breaking the
dependencies
keyword in this draft, and I think this is a much better solution for both clarity and migration support.Thoughts? Paging some folks who weighed in on other
dependencies
issues: @erayd @markchart @pipobscureThe text was updated successfully, but these errors were encountered: