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

add support for $defs instead of definitions. #338

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rpatters1
Copy link

One of the changes for the 1020-12 schema specification (#153) is the keyword $defs instead of definitions. This PR implements that change.

With this change in place I can load the MNX schema and correctly validate MNX files against it. However, I am a newbie here, so I welcome comments on how to improve it.

@LecrisUT
Copy link
Collaborator

I did not inspect the code, but I believe this needs an implementation of the 2020-12 draft to recognize it as an acceptable json schema version. Right now only up to Draft7 are supported afaik

@rpatters1
Copy link
Author

So, either this project is stuck at draft-07 and gradually fades to irrelevance, or someone takes on the changes needed to update it to 2020-12. Barring participation by the OP (which he has said he won't do), it seems to me like a piecemeal approach is more practical than a grand all-encompassing project. But maybe that's my too-practical side coming out.

@LecrisUT
Copy link
Collaborator

Piece-by-piece I think it's an ok approach. The thing that strikes me is that it would need a flag to check if it's draft 2020-12. I don't know exactly where that would be defined though, maybe that is not defined yet?

@rpatters1
Copy link
Author

Checking a flag is an excellent idea. I will see what I can figure out about that. Taking that as given, it would helpful to know if my trivial change in this PR is sufficient to support $defs. (The good news is that 2020-12 still allows definitions.)

I also noticed the use of $defs in some of the test files. Perhaps those are supposed to fail.

@rpatters1
Copy link
Author

Just doing some checking, it seems like there is no flag currently. Two approaches occur to me:

Read it from $schema

Advantages

  • no action required by the caller

Disadvantages

  • the schema must supply a $schema setting. Many schemas do not.
  • not very future proof. We don't know what future schemas are coming down the pike, but supposedly they will be backwards compatible. But if we read it, we will only find ones that specify 2020-12.

Specify the version of checking you want when you create the schema instance.

Advantages

  • no $schema value need be supplied.
  • future-proof. The caller can specify the supported schema versions in an enum, which will default to draft-07. As long as future schema specifications are backward compatible with 2020-12, the 2020-12 enum value will still work.

Disadvantages

  • the caller must decide and hard-code it.

@LecrisUT
Copy link
Collaborator

I would say read it from schema if you did not provide a class with a specific validator that you want to override. I.e. have a factory that creates the schema-specific validator by parsing the key $schema, and if you don't specify your own schema-specific validator, call the factory. That should be the best of both world right, and it should future-proof for custom vocabularies.

@pboettch
Copy link
Owner

I'm for updating the library to the latest standard, even if incompatible with older drafts.

Support for older versions can be maintained on branches, if needed. People should be encouraged to migrate their schema to newer versions of the standard.

This PR is acceptable as is I think. No need for flagging. In #153 I created an issue for support 2020-12. And ask for volunteers in helping.

Maybe the moment has come? To work on it as a team?

@rpatters1
Copy link
Author

I'm for updating the library to the latest standard, even if incompatible with older drafts.

In that case, perhaps it should check for $defs first. I will make that change when I get back to my desk.

Also, what about a test? Does it make sense to modify one or more of the existing ones or to create a new one?

@LecrisUT
Copy link
Collaborator

I don't know if supporting only one schema like that would be good, since it limits hacking and having to work on supporting each schema as a whole instead of gradually working on it with experimental support.

I'm not sure which approach would be best right now for supporting multiple schemas, but here are a few:

  • enum the official schemas and check feature support by comparing the version number
  • abstract each schema's feature, e.g. resolve_ref() and implement relevant logics for each schema
  • have trait classes/singleton and query support for each feature there

Each of these would have their pros and cons for hacking and ease of developing, but I don't think any of them would have crucial flaws that the user would be blocked from hacking because we chose one style over another.

I can start devoting some time now for this project, so I'll pick up the dangling PRs I had probably next month.

@rpatters1
Copy link
Author

I don't know if supporting only one schema like that would be good, since it limits hacking and having to work on supporting each schema as a whole

In the context of this PR, at least, the change is backwards compatible with draft-07. Other features of 2020-12 will not be, of course. Then the issue could become more acute.

@rpatters1
Copy link
Author

It sounds like @LecrisUT has a plan for flagging. If @pboettch is cool with this as-is, I will let someone with a "higher paygrade" decide whether to merge this PR. I made the change I wanted to, which is to check for $defs before checking for definitions.

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

Successfully merging this pull request may close these issues.

3 participants