Skip to content

Fix bug in current registration of validators and address #417 #430

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

Merged
merged 1 commit into from
Jul 8, 2018

Conversation

agoose77
Copy link
Contributor

@agoose77 agoose77 commented Jul 6, 2018

Alternative solution to #417 (fixes #417)
Currently, the validator registration is hardcoded to use the deprecated "id" keyword, rather than using the new "id_of" function passed to create. This means that only draft 3 and 4 are currently registered. This is not visibly a bug at present, because there is only one other schema, and the fallback mechanism selects the most recent. However, for good practice we should still register the draft 6 schema.

This PR adds a Validator.ID_OF staticmethod, and call it from the registrar.

@agoose77 agoose77 changed the title Fix bug in current registration of validators Fix bug in current registration of validators and address #GH-417 Jul 6, 2018
@agoose77 agoose77 changed the title Fix bug in current registration of validators and address #GH-417 Fix bug in current registration of validators and address #417 Jul 6, 2018
@robherring
Copy link
Contributor

Presumably @Julian will want test cases for this. You can extract the ones I wrote from #417 if you want.

@agoose77
Copy link
Contributor Author

agoose77 commented Jul 6, 2018

Hmm, do you think we need to test this? I suppose we could check all the current drafts exist in the registry.
If you have those tests already, that would be very helpful!

@Julian
Copy link
Member

Julian commented Jul 8, 2018

Yes, every change needs tests that cover every part of it :)

@robherring I think I prefer this way a bit, but yours has tests :), so will see if we can combine those together.

@Julian
Copy link
Member

Julian commented Jul 8, 2018

OK, merged! Thanks.

@Julian Julian merged commit 7a8634e into python-jsonschema:master Jul 8, 2018
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