Skip to content

Conversation

@gonvaled
Copy link
Contributor

Very simple implementation. No test case added since it is covered by TestValidate.

@gazpachoking
Copy link
Contributor

Maybe it should take a default keyword arg rather than have Draft4Validator hard set as the default?

@gonvaled
Copy link
Contributor Author

Draf4Validator was the default before, and stays as default in this new commit. Anyway: how are you going to provide a default? Do you really want to import Draft4Validator in client code? Or maybe provide a new validator class?

It would not be difficult to do, though:

def validator_for(schema, default_validator=Draft4Validator):
    return meta_schemas.get(schema.get("$schema", ""), default_validator)

I can do that if required.

@gazpachoking
Copy link
Contributor

Yeah, I was just thinking that if we were making validator_for public now maybe it should be an option. I'll defer to whatever @Julian things though.

@Julian
Copy link
Member

Julian commented May 17, 2013

The reason default is good is because Draft4Validator is an arbitrary default, not one specified by the spec (right now it's just because it's the newest version, but if draft 5 is released tomorrow [not likely] we'd still likely have draft 4 as the default for backwards compat for a while), and clients should be expected to make that decision as well.

There probably is a secondary use case here for "I want the default that jsonschema would have provided", and I think None is unfortunately going to be a thing someone might actually want as a default for "I can't figure it out", so maybe validator_for(schema, default=_unset) would be the thing.

Also it needs at least two tests (one for when a validator is found from $schema, and one for when it isn't, and now I guess one for _unset) :D.

And it'd be nice if even though there isn't any prose there now if you added at least an .. autofunction validator_for:: to the docs in docs/creating.rst, since I think at the moment that will probably be the most likely place for it to be documented.

Thanks for working on this!

@gonvaled gonvaled closed this May 21, 2013
@gonvaled
Copy link
Contributor Author

Maybe you mean something like this: #113

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