-
-
Notifications
You must be signed in to change notification settings - Fork 590
Widened type interface #374
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
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
644259b
The new type interface: a TypeChecker class
bsmithers 7c41d0b
Updated docs with new TypeChecker interface
bsmithers 9c1c265
Allow _type_checkers to be def'd as a dict at init
bsmithers 89a6739
Fix the default_types deprecation
bsmithers 9467cbf
Revert "Allow _type_checkers to be def'd as a dict at init"
bsmithers 34227e8
Use attr.ib's convert arg to generate the pmap
bsmithers 30bec1a
Add checker instance to type checking functions
bsmithers c202c48
Minor changes addressing PR review issues
bsmithers f7ab18e
Merge remote-tracking branch 'upstream/master' into types_draft4
bsmithers e2264e5
Simplify redefine_many.
bsmithers File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,205 @@ | ||
import numbers | ||
|
||
import attr | ||
import pyrsistent | ||
|
||
from jsonschema.compat import str_types, int_types, iteritems | ||
from jsonschema.exceptions import UndefinedTypeCheck | ||
|
||
|
||
def is_array(checker, instance): | ||
return isinstance(instance, list) | ||
|
||
|
||
def is_bool(checker, instance): | ||
return isinstance(instance, bool) | ||
|
||
|
||
def is_integer(checker, instance): | ||
# bool inherits from int, so ensure bools aren't reported as ints | ||
if isinstance(instance, bool): | ||
return False | ||
return isinstance(instance, int_types) | ||
|
||
|
||
def is_null(checker, instance): | ||
return instance is None | ||
|
||
|
||
def is_number(checker, instance): | ||
# bool inherits from int, so ensure bools aren't reported as ints | ||
if isinstance(instance, bool): | ||
return False | ||
return isinstance(instance, numbers.Number) | ||
|
||
|
||
def is_object(checker, instance): | ||
return isinstance(instance, dict) | ||
|
||
|
||
def is_string(checker, instance): | ||
return isinstance(instance, str_types) | ||
|
||
|
||
def is_any(checker, instance): | ||
return True | ||
|
||
|
||
@attr.s(frozen=True) | ||
class TypeChecker(object): | ||
""" | ||
A ``type`` property checker. | ||
|
||
A :class:`TypeChecker` performs type checking for an instance of | ||
:class:`Validator`. Type checks to perform are updated using | ||
:meth:`TypeChecker.redefine` or :meth:`TypeChecker.redefine_many` and | ||
removed via :meth:`TypeChecker.remove` or | ||
:meth:`TypeChecker.remove_many`. Each of these return a new | ||
:class:`TypeChecker` object. | ||
|
||
Arguments: | ||
|
||
type_checkers (dict): | ||
|
||
The initial mapping of types to their checking functions. | ||
""" | ||
_type_checkers = attr.ib(default={}, convert=pyrsistent.pmap) | ||
|
||
def is_type(self, instance, type): | ||
""" | ||
Check if the instance is of the appropriate type. | ||
|
||
Arguments: | ||
|
||
instance (object): | ||
|
||
The instance to check | ||
|
||
type (str): | ||
|
||
The name of the type that is expected. | ||
|
||
Returns: | ||
|
||
bool: Whether it conformed. | ||
|
||
|
||
Raises: | ||
|
||
:exc:`jsonschema.exceptions.UndefinedTypeCheck`: | ||
if type is unknown to this object. | ||
""" | ||
try: | ||
fn = self._type_checkers[type] | ||
except KeyError: | ||
raise UndefinedTypeCheck(type) | ||
|
||
return fn(self, instance) | ||
|
||
def redefine(self, type, fn): | ||
""" | ||
Redefine the checker for type to the function fn. | ||
|
||
Arguments: | ||
|
||
type (str): | ||
|
||
The name of the type to check. | ||
|
||
fn (callable): | ||
|
||
A function taking exactly two parameters - the type checker | ||
calling the function and the instance to check. The function | ||
should return true if instance is of this type and false | ||
otherwise. | ||
|
||
Returns: | ||
|
||
A new :class:`TypeChecker` instance. | ||
|
||
""" | ||
return self.redefine_many({type:fn}) | ||
|
||
def redefine_many(self, definitions=()): | ||
""" | ||
Redefine multiple type checkers. | ||
|
||
Arguments: | ||
|
||
definitions (dict): | ||
|
||
A dictionary mapping types to their checking functions. | ||
|
||
Returns: | ||
|
||
A new :class:`TypeChecker` instance. | ||
|
||
""" | ||
definitions = dict(definitions) | ||
type_checkers = self._type_checkers.update(definitions) | ||
return attr.evolve(self, type_checkers=type_checkers) | ||
|
||
def remove(self, type): | ||
""" | ||
Remove the type from the checkers that this object understands. | ||
|
||
Arguments: | ||
|
||
type (str): | ||
|
||
The name of the type to remove. | ||
|
||
Returns: | ||
|
||
A new :class:`TypeChecker` instance | ||
|
||
Raises: | ||
|
||
:exc:`jsonschema.exceptions.UndefinedTypeCheck`: | ||
if type is unknown to this object | ||
|
||
""" | ||
return self.remove_many((type,)) | ||
|
||
def remove_many(self, types): | ||
""" | ||
Remove multiple types from the checkers that this object understands. | ||
|
||
Arguments: | ||
|
||
types (iterable): | ||
|
||
An iterable of types to remove. | ||
|
||
Returns: | ||
|
||
A new :class:`TypeChecker` instance | ||
|
||
Raises: | ||
|
||
:exc:`jsonschema.exceptions.UndefinedTypeCheck`: | ||
if any of the types are unknown to this object | ||
""" | ||
evolver = self._type_checkers.evolver() | ||
|
||
for type_ in types: | ||
try: | ||
del evolver[type_] | ||
except KeyError: | ||
raise UndefinedTypeCheck(type_) | ||
|
||
return attr.evolve(self, type_checkers=evolver.persistent()) | ||
|
||
|
||
draft3_type_checker = TypeChecker({ | ||
u"any": is_any, | ||
u"array": is_array, | ||
u"boolean": is_bool, | ||
u"integer": is_integer, | ||
u"object": is_object, | ||
u"null": is_null, | ||
u"number": is_number, | ||
u"string": is_string | ||
}) | ||
|
||
draft4_type_checker = draft3_type_checker.remove(u"any") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ech, so I didn't know whether this was intentional or not, but now I see that there's some infinite recursion nonsense here when you upcall...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there? In order to use the
is_my_int
, a user must generate a newTypeChecker
instance (via, e.g.redefine
), which is then given to a new validator (viacreate
orextend
). In other words:Is not changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This was a half-baked comment that I just wrote to make sure I didn't forget to address it, sorry for the missing context -- it's the following:)
Not in the way you wrote it, but in the way I thought you mean to write it, which was:
return checker.is_type(instance, "number") or isinstance(instance, MyInteger)
That will do nasty infinite things because it calls itself, but I think the expectation there is clearly to upcall to the original definition of number.
And the above case was basically my motivation for suggesting we include the
checker
as one of the two arguments to the function (because it allows for forward extension).The infinite recursion case though makes that a bit more complicated, it means there's a different thing you do if you're overriding the same type and want to upcall as if you're trying to access another type checker.
I think this needs a bit more thought -- my first reaction when I realized the above was "maybe we should pass in a third argument too, the original function from the checker that was redefined", but I think we just need to think through the 2 use cases again and make sure this API makes sense (the use case where you're accessing another checker, and the one where you're trying to upcall to the original one).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies as I think we could have had this conversation earlier. I was concerned about attempted infinite recursion too but liked the ability to reference another type via
checker
.The third argument doesn't immediately feel right to me - mostly because of the awkward basecase of a null function (in fact its not a null function, it's one that would raise an exception for an unknown type).
Will have a think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for going AWOL on this - turns out changing jobs and moving takes up a fair bit of time.
As far as I remember, this point was the main remaining issue. I think I am in favour of the current API as a balance of simplicity and power. To me, the biggest benefit for passing in the type checker object (i.e. the current API) is for the ease of custom types, e.g. "string_or_int". In addition, it feels OK to be pretty explicit about which type checking function you are upcalling to in the case of a type redefinition.
Passing in both the object and function that is being overwritten feels like we are providing a somewhat bizarre OO inheritance mechanism, which I dislike.
What do you think? Perhaps we simply add a warning to the documentation on the potential for infinite recursion? If you're happy with that I'll sort a PR for this and the other minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely no need to apologise, trust me staying at the same job and not moving is also pretty killer :(
I think the warning might be the thing, but I wanna reread my own comment again and see if a months time has given me any subconscious enlightenment (I doubt it...)