Automatically create metaschemas when accessing JSONSchema.metaschema #71
Conversation
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #71 +/- ##
==========================================
+ Coverage 90.98% 91.42% +0.43%
==========================================
Files 21 21
Lines 1986 2006 +20
Branches 422 427 +5
==========================================
+ Hits 1807 1834 +27
+ Misses 118 113 -5
+ Partials 61 59 -2
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
This will be used to test several forthcoming changes.
This change allows the tests added in PR json-schema-org/JSON-Schema-Test-Suite#646 to pass, in accordance with §9.3.1 of the spec, "Detecting a Meta-Schema". * Catalog.get_metaschema() is added, and is analogous in behavior to Catalog.get_schema(), relying on Catalog.create_metaschema() * It is called from the now-cached JSONSchema.metaschema property, to keep the parallel with references which are only resolved when used * Catalog.create_metaschema() now returns the created metaschema to avoid having to immediately look it up again, as does Catalog.create_vocabulary() * The core_vocabulary parameters have become default_core_vocabulary parameters to allow not knowing the core in advance * The Metaschema constructor now looks in "$vocabulary" for a vocabulary URI matching r'^https://json-schema\.org/draft/[^/]*/core' and if it finds a unique match, uses that instead of the default * Lack of a recognizable core vocabulary still results in a JSONSchemaError exception if no default is provided
|
Having spent a great deal more time in all the relevant areas of the code, I haven't come up with any better approach, so I've moved this out of draft PR status. |
Following 0b1108b, we just need to handle the initially non-existent '__meta__' cache, and then explicit calls to `Catalog.create_metaschema()` can generally be removed.
|
Nice work, @handrews! Do you mind if I push to this branch? |
|
@marksparkza Thanks! I think I opened this PR with the "allow maintainer to make changes" - thanks for checking but feel free to add commits. What workflow do you have in mind? Are you looking to discuss those additional commits? (Just trying to understand the difference between what you want to do here and merging and then adding things separately). |
|
Or did you mean create this branch in your repo rather than going straight to main? I'm also totally fine with that. |
|
Automated meta-schema creation is a big win, and I think it's worth rounding off this PR by removing boilerplate There are one or two changes that I'd still like to take a closer look at, so I may add further comments following the additional commits. Likewise, please feel free to comment on or query any changes that I make to your code. Once this PR is merged we're in a good position to publish a very long awaited v0.10, so I'm going to go ahead with that and work through the remaining PRs for the next version increment. |
|
@marksparkza Sounds great! I will try to finish the PR for the I have some other things in the works for draft-next support, but that can wait (and TBH I'm about to not have as much time avaialble so they'd probably take a while before I got around to them). |
@marksparkza I think I slightly misunderstood you although it doesn't really change anything. Just to be sure I understand: You plan to merge this PR once it's finished and immediately release 0.10.0, correct? If so, I would recommend also merging the very simple PRs #72 (putting the not-found schema file name in the I'm perfectly happy with the rest (including the BTW, when you push the 0.10.0 release, I'll rebase the other PRs and rework their CHANGELOG updates accordingly. |
Yes, that's the plan. I'd prefer to merge this PR as is, and subsequently look at 72 and 73 for a 0.10.1 patch. I do appreciate that they are quite straightforward, but they're also relatively insignificant and shouldn't hold up or force a rebase of 71. Having said that, we're looking at a timescale of days rather than months for 0.10.1.
Thanks, much appreciated. Please just do 72 and 73, and the rest once 0.10.1 is out. I'll be happy to include the
Yes, absolutely! |
|
@marksparkza thanks for the explanation - that all sounds fantastic! Yes, a 0.10.1 makes sense — I think I am too used to projects that are reluctant to do quick follow-up releases so that simply didn't occur to me. |
3 participants
Fixes #68 (2nd try, now with much more testing!) , although I'm submitting it as a draft to indicate that I'm not particularly confident that this is the ideal way to solve it. But it's a starting point, at least.
This change allows the tests added in PR json-schema-org/JSON-Schema-Test-Suite#646 (updated here in PR #67) to pass, in accordance with §9.3.1 of the spec, "Detecting a Meta-Schema".
Catalog.get_metaschema()is added, and is analogous in behavior toCatalog.get_schema(), checking the__meta__cache directly and then relying onCatalog.create_metaschema()to avoid needlessly creating bothJSONSchemaandMetaschemainstances.JSONSchema.metaschemaproperty, to keep the parallel with references which are only resolved when used.Catalog.create_metaschema()now returns the created metaschema to avoid having to immediately look it up again, as doesCatalog.create_vocabulary()core_vocabularyparameters have becomedefault_core_vocabularyparameters to allow not knowing the core in advance*The
Metaschemaconstructor now looks in"$vocabulary"for a vocabulary URI matchingr'^https://json-schema\.org/draft/[^/]*/core$and if it finds a unique match, uses that instead of the defaultMetaschematest cases, so theCatalog-related ones went into that test file and the constructor ones went into a newtest_metaschema.pyNote that the fixture is a separate commit because it is shared with the next PR I'm about to post.