Skip to content

Return the instance from validate #462

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

Closed
wants to merge 5 commits into from

Conversation

bcb
Copy link

@bcb bcb commented Sep 22, 2018

The validate function currently returns None. Returning the instance allows the user to use validate in a function chain, for example:

try:
    return process(validate(deserialize(string)))
except jsonschema.ValidationError:
    ...

bcb added 5 commits September 22, 2018 13:06
The validate function currently returns None. Returning the instance
allows the user to use validate in a function chain.
@Julian
Copy link
Member

Julian commented Sep 23, 2018

Hey -- thanks for the PR, it's appreciated.

It's a bit late in jsonschema's life to make this kind of change. If it were being rewritten from scratch it's very likely this is how this would have looked, and someday if/when Seep gets some love, it has an API like this, but at this point I don't think I want to take this unfortunately.

There's a few reasons other than just inertia -- they're minor but I'll mention them -- one is that in exchange for being able to write the code you put up there (which I'd call only a millimeter better than just using separate lines), someone now will overextend their except block and try to cover exceptions from all 3 functions at the same time -- often incorrectly so, and then be hit by bugs when exceptions they don't expect come from functions they don't expect. ValueError is a great example, since that is the awful exception the json module blesses us with and is a good possibility for your deserialize. There's also another minor one that typically stuff that returns things gets named in past tense, so validated here instead of validate. And lastly, I'm sure for better or worse someone has (incorrectly) written a line of code that looks like if jsonschema.validate(...) is None in attempting to test whether things passed validation, which turns this into a very slight issue of backwards compatibility, since None is a perfectly valid instance. These all (even together) were probably ignorable, but pointing them out as well since they do at least come to mind.

Overall I think this is really appreciated, and definitely a reasonable change to propose, but I'd push you towards contributing within Seep if you're trying to make something that looks like this happen!

@Julian Julian closed this Sep 23, 2018
@bcb
Copy link
Author

bcb commented Sep 23, 2018

Thanks for the thought-out reply.

Worth mentioning that since Python 3.5 the json module raises a more specific JSONDecodeError.

For now I can write my own wrapper around jsonschema.validate.

@Julian
Copy link
Member

Julian commented Sep 23, 2018 via email

Julian added a commit that referenced this pull request Apr 13, 2021
15ec577 Merge pull request #471 from json-schema-org/ether/id-anchor-in-enum
9f97865 test for confusing not-identifiers in enums
0f7ecd4 Merge pull request #475 from marksparkza/marksparkza-patch-1
783d22a Add jschon
fc68499 Merge pull request #472 from json-schema-org/ether/unevaluatedProperties_uncles
ed4cf5f more test cases for unevaluatedItems, unevaluatedProperties
d0d814d Merge pull request #469 from json-schema-org/ether/ipv4-vulnerability
7ca5f36 reject ipv4 strings with an octet with a leading zero
8e1e1c1 fix spelling error in test descriptions
77f1d10 Merge pull request #462 from jdesrosiers/dynamic-ref-tests
72a32fe Merge pull request #468 from json-schema-org/ether/combine-test-cases
0c48ffb Merge pull request #453 from notEthan/float-overflow-d4-int
76a4ba0 these test cases can be combined since the schemas are the same
cd73775 Merge pull request #464 from json-schema-org/ether/format-by-default-always-validates
043dc63 by default, "format" only annotates, not validates
3c45b81 Merge pull request #460 from amosonn/remove-remotes-from-script
b09e48d Fix $ref with siblings in pre-2019-09 tests
ff9f22e Add tests for $dynamicRef/$dynamicAnchor
0faaf09 Fix refs to Draft 2019-09 schema to be refs to 2020-12
ebbcbc8 Use flask to server remotes directly
bb98b03 Remove remotes from bin/jsonschema_suite
fcae732 Merge pull request #455 from jdesrosiers/bootstrap-202012
e002409 Update tests for 2020-12
405b3fb Copy 2019-09 tests to bootstrap 2020-12 tests
1636a22 draft4 float-overflow instance may be considered not an integer
8daea3f Merge pull request #451 from json-schema-org/ether/more-relative-json-pointer
69fe40f some more relative-json-pointer tests
6505944 Merge pull request #450 from json-schema-org/ether/recursiveRef-dynamic-path
afd0cd3 Move content* keyword tests to non-optional
e2b2a4b Change all content* keyword tests to always validate
8999eae $recursiveRef example demonstrating dynamic nature of the resolution scope
f47003f fix duplicate test description
bcf1dc8 Merge pull request #391 from ether/recursiveRef (rebased, squashed)
3d88f34 test $recursiveRef + $recursiveAnchor
3b79a45 Merge pull request #418 from ChALkeR/chalker/contentSchema
29f609b Add tests for contentSchema

git-subtree-dir: json
git-subtree-split: 15ec577f5ddee0115319f4e7f856cd57567a9c78
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.

2 participants