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

Rename $recursive* to $dynamic*, make it work with normal anchors / plain name fragments instead of base URI switching #930

Merged
merged 10 commits into from
Jul 17, 2020

Conversation

handrews
Copy link
Contributor

@handrews handrews commented May 20, 2020

Closes #909 (improve/simplify/rename $recursive*)
Closes #868 (Let's not require runtime base URI tracking)
Also addresses 2/3 or #809 (overly long section names) but the other 1/3 is not in this part of the spec so I'll close that separately.

Instead of using a boolean (which never really fit with how the similarly named "$anchor" worked, and meant only one dynamic resolution was possible at a time) and messing with base URIs and re-resolving URI-references (a topic that creates plenty of confusion even on its own), use a fragment name the same as "$anchor" and make "$dynamicRef" work by just looking for the outermost resource with a matching "$dynamicAnchor" fragment name.

This stil requires that the initial "$dynamicRef" target URI be a valid URI, with a fragment created by "$dynamicAnchor", in order to produce the dynamic behavior. This ensures that it is always clear which schemas are dynamic extension points and which are not.

@handrews
Copy link
Contributor Author

Note for those who saw the note that I removed from the main description: I decided not to try to do overly clever things with the hyper-schema meta-schemas, and added a commit that just updates them (and the other meta-schemas) more or less as they are. I might come back to the idea (which involved both the hyper-schema meta-schema and the links schema being dynamically extended) but TBH it's pretty complicated and of questionable value.

Let's focus on the essentials for now.

Copy link
Member

@Relequestual Relequestual left a comment

Choose a reason for hiding this comment

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

This looks good and looks like it resolved the issues it claims to resolve.

The was one section where I'm not sure I fully understand, even with the example.

Given the changes are clear to me apart from that small section, once explained, it may need a small tweak language wise.

jsonschema-core.xml Show resolved Hide resolved
@handrews
Copy link
Contributor Author

Also, I'll leave this open for the full 14-day period for major changes, but once approved I'll work on #849 and #857 on the assumption that this or something very much like it will get merged. That work (about schema loading) needed to account for whether the old runtime base URI switching was present, which made the loading+runtime process substantially more complex.

Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

Look good to me. Just that one typo needs fixing.

jsonschema-core.xml Outdated Show resolved Hide resolved
Preliminary work for revamping these keywords.
Instead of using a boolean (which never really fit with how
the similarly named "$anchor" worked, and meant only one
dynamic resolution was possible at a time) and messing
with base URIs and re-resolving URI-references (a topic
that creates plenty of confusion even on its own),
use a fragment name the same as "$anchor" and make "$dynamicRef"
work by just looking for the outermost resource with a matching
"$dynamicAnchor" fragment name.

This stil requires that the initial "$dynamicRef" target URI
be a valid URI, with a fragment created by "$dynamicAnchor",
in order to produce the dynamic behavior.  This ensures
that it is always clear which schemas are dynamic extension
points and which are not.
This updates for the change of $recursiveRef (with a boolean)
to $dynamicAnchor (with a plain name fragment), and provides
a more detailed example of how the dynamic scope is used.
Replace $recursiveAnchor: true with $dynamicAnchor: "meta"
and update $recursiveRef to $dynamicRef accordingly.
jsonschema-core.xml Outdated Show resolved Hide resolved
jsonschema-core.xml Outdated Show resolved Hide resolved
/dyanmic/dynamic/
/theses/these/
@Relequestual
Copy link
Member

I may have fix / fixed typo... If only I had been consistent in that... =]

@Relequestual
Copy link
Member

@karenetheridge can you review this again please? =]

jsonschema-core.xml Outdated Show resolved Hide resolved
jsonschema-core.xml Show resolved Hide resolved
jsonschema-core.xml Outdated Show resolved Hide resolved
links.json Show resolved Hide resolved
meta/applicator.json Show resolved Hide resolved
@Relequestual
Copy link
Member

Relequestual commented Jul 15, 2020

@handrews If you approve of the changes made since your initial PR, we're basically ready.

I note that after seeing #940, the output schema needs to be updated to reflect, but that can be a separate issue after this one is merged (because LGTM)

@handrews
Copy link
Contributor Author

@Relequestual looks great. I had to stare at $dyanmic for a long time before I could even figure out what changed 😂
I'll go ahead and merge.

@handrews handrews merged commit 1d9c0d3 into json-schema-org:master Jul 17, 2020
@handrews handrews deleted the dynamic branch September 26, 2020 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants