-
-
Notifications
You must be signed in to change notification settings - Fork 592
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
Changes from 2 commits
644259b
7c41d0b
9c1c265
89a6739
9467cbf
34227e8
30bec1a
c202c48
f7ab18e
e2264e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,10 +29,16 @@ Creating or Extending Validator Classes | |
will have :func:`validates` automatically called for the given | ||
version. | ||
|
||
:argument dict default_types: a default mapping to use for instances | ||
of the validator class when mapping between JSON types to Python | ||
types. The default for this argument is probably fine. Instances | ||
can still have their types customized on a per-instance basis. | ||
:argument dict default_types: Deprecated. Please use the type_checker | ||
argument instead. | ||
|
||
If set, it provides mappings of JSON types to Python types that will | ||
be converted to functions and redefined in this object's TypeChecker | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Period at the end please, and either "type checker" (i.e. not the class name) or ":class: |
||
|
||
:argument jsonschema.TypeChecker type_checker: an instance | ||
of :class:`TypeChecker`, whose :meth:`is_type` will be called to | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It won't insert the word "method", so you have to say ":meth: |
||
validate the :validator:`type` property If unprovided, a default | ||
:class:`TypeChecker` will be created, with no support types. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think actually we can leave off the "if unprovided" entirely here and just rely on people looking at the newer property (which we've already pointed them to) for info on default behavior. |
||
|
||
:returns: a new :class:`jsonschema.IValidator` class | ||
|
||
|
@@ -59,6 +65,10 @@ Creating or Extending Validator Classes | |
|
||
:argument str version: a version for the new validator class | ||
|
||
:argument jsonschema.TypeChecker type_checker: an instance | ||
of :class:`TypeChecker`. If unprovided, the existing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I try not to repeat instance-of-class kind of info in the actual prose (because it's already in the |
||
:class:`TypeChecker` will be used. | ||
|
||
:returns: a new :class:`jsonschema.IValidator` class | ||
|
||
.. note:: Meta Schemas | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,7 @@ Contents: | |
references | ||
creating | ||
faq | ||
types | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason to add this as an entirely new document? If not might just be reasonable to put this in the |
||
|
||
|
||
Indices and tables | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
.. currentmodule:: jsonschema | ||
|
||
============= | ||
Type Checking | ||
============= | ||
|
||
Each :class:`IValidator` has an associated :class:`TypeChecker`. The | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, half baked thought, but I guess in retrospect it's not even obvious that every validator has the Definitely not saying to change anything, but feel the need to note that uncomfortableness somewhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting use case. I agree that it seems pretty core to JSON schema. Nonetheless I've reworded the docs slightly. It is slightly clunky but one would simply not provide a |
||
TypeChecker provides an immutable mapping between names of types and | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would recommend "type checker" again here. |
||
functions that can test if an instance is of that type. The defaults are | ||
suitable for most users - each of the predefined Validators (Draft3, Draft4) | ||
has a :class:`TypeChecker` that can correctly handle that draft. | ||
|
||
See :ref:`validating-types` for an example of providing a custom type check. | ||
|
||
.. autoclass:: TypeChecker | ||
:members: | ||
|
||
.. autoexception:: jsonschema.exceptions.UndefinedTypeCheck | ||
|
||
Raised when trying to remove a type check that is not known to this | ||
TypeChecker. Internally this is also raised when calling | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't really get a choice on the "internally" I think.
|
||
:meth:`TypeChecker.is_type`, but is caught and re-raised as a | ||
:class:`jsonschema.exceptions.UnknownType` exception. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,10 +32,12 @@ classes should adhere to. | |
will validate with. It is assumed to be valid, and providing | ||
an invalid schema can lead to undefined behavior. See | ||
:meth:`IValidator.check_schema` to validate a schema first. | ||
:argument types: Override or extend the list of known types when | ||
:argument types: Deprecated. Instead, create a custom TypeChecker | ||
and extend the validator. See :ref:`validating-types` for details. | ||
|
||
If used, this overrides or extends the list of known type when | ||
validating the :validator:`type` property. Should map strings (type | ||
names) to class objects that will be checked via :func:`isinstance`. | ||
See :ref:`validating-types` for details. | ||
:type types: dict or iterable of 2-tuples | ||
:argument resolver: an instance of :class:`RefResolver` that will be | ||
used to resolve :validator:`$ref` properties (JSON references). If | ||
|
@@ -48,8 +50,10 @@ classes should adhere to. | |
|
||
.. attribute:: DEFAULT_TYPES | ||
|
||
The default mapping of JSON types to Python types used when validating | ||
:validator:`type` properties in JSON schemas. | ||
Deprecated. Under normal usage, this will be an empty dictionary. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't make this an empty dictionary, it has to preserve the existing behavior at least for We also need to have accessing the attribute warn with one of the warnings you've added (thanks!) We can either use a property for that, or look at one of the libraries that are focused around "deprecation" itself if it starts to get annoying. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. This is partly a wording issue but I'm also going to revisit that in the code. Currently There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This isn't quite a trivial as first sounds - So we could do one of the fudges for class properties, but is it worth it? Any access to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You'd need a metaclass yeah (which was what I meant by "a deprecation library", though the one I was intending I could have sworn had something for this, but I don't see it) Besides the fact that it's uncomfortable to try to guess what code exists (I've been surprised / horrified at some of the jsonschema-using-code I've seen :) ), it seems like someone who even sees the warning from one of those two places is not-totally-unlikely to only half fix things (by just continuing to use So I'm not definitely convinced we need it, but yeah I'd lean towards it being worthwhile, despite being annoying :/ If it presents a real annoyance though let me know and we should find something that does this. It's one of the things that is super super frustrating about Python in 2017 -- it should have a good story for this but unfortunately still doesn't... |
||
|
||
If set, it provides mappings of JSON types to Python types that will | ||
be converted to functions and redefined in this object's TypeChecker | ||
|
||
.. attribute:: META_SCHEMA | ||
|
||
|
@@ -62,6 +66,10 @@ classes should adhere to. | |
that validate the validator property with that name. For more | ||
information see :ref:`creating-validators`. | ||
|
||
.. attribute:: TYPE_CHECKER | ||
A :class:`TypeChecker` that can be used validating :validator:`type` | ||
properties in JSON schemas. | ||
|
||
.. attribute:: schema | ||
|
||
The schema that was passed in when initializing the object. | ||
|
@@ -134,10 +142,7 @@ Validating With Additional Types | |
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
Occasionally it can be useful to provide additional or alternate types when | ||
validating the JSON Schema's :validator:`type` property. Validators allow this | ||
by taking a ``types`` argument on construction that specifies additional types, | ||
or which can be used to specify a different set of Python types to map to a | ||
given JSON type. | ||
validating the JSON Schema's :validator:`type` property. | ||
|
||
:mod:`jsonschema` tries to strike a balance between performance in the common | ||
case and generality. For instance, JSON Schema defines a ``number`` type, which | ||
|
@@ -152,24 +157,24 @@ more general instance checks can introduce significant slowdown, especially | |
given how common validating these types are. | ||
|
||
If you *do* want the generality, or just want to add a few specific additional | ||
types as being acceptable for a validator object, :class:`IValidator`\s have a | ||
``types`` argument that can be used to provide additional or new types. | ||
types as being acceptable for a validator object, then you should update an | ||
existing :class:`TypeChecker` or create a new one. You may then create a new | ||
:class:`IValidator` via :meth:`extend`. | ||
|
||
.. code-block:: python | ||
|
||
class MyInteger(object): | ||
... | ||
pass | ||
|
||
def is_my_int(instance): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question which maybe was more obvious when writing this: maybe these functions should take both the instance and also the checker itself, rather than needing to "manually" pull that off? (E.g., that'd allow for more easily reusing functions across drafts) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I follow this one - seems it will end up quite circular as each of these functions is already attached to a checker. I also wonder if it would be a performance hit, since each type check would then also be doing a comparison on type checker object? Or maybe I misunderstood. For reusing across drafts - that's already handled through redefinition of types->functions, no? As in, most of the type checks remain the same and a new draft only redefines a subset. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They're attached to a checker, but they don't know what the "current" checker knows. E.g., imagine someone wants to write a checker that satisfies "number or string" -- they can't do that under the "current" definition of number of a type checker, whatever that might be after someone changes things. I.e., compare:
vs.
(This is a toy example, but hopefully the point is clear enough now at least? If not lemme know) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yeah I get it now. I think that's a worthwhile change. It's particularly useful for custom types as when you're overwriting the checking function for an existing type you'll no longer be able to access the original via |
||
return Draft3Validator.TYPE_CHECKER.is_type(instance, "number") or \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Parens instead of |
||
isinstance(instance, MyInteger) | ||
|
||
type_checker = Draft3Validator.TYPE_CHECKER.redefine("number", is_my_int) | ||
|
||
Draft3Validator( | ||
schema={"type" : "number"}, | ||
types={"number" : (numbers.Number, MyInteger)}, | ||
) | ||
CustomValidator = extend(Draft3Validator, type_checker=type_checker) | ||
validator = CustomValidator(schema={"type" : "number"}) | ||
|
||
The list of default Python types for each JSON type is available on each | ||
validator object in the :attr:`IValidator.DEFAULT_TYPES` attribute. Note | ||
that you need to specify all types to match if you override one of the | ||
existing JSON types, so you may want to access the set of default types | ||
when specifying your additional type. | ||
|
||
.. _versioned-validators: | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,206 @@ | ||
import numbers | ||
|
||
import attr | ||
import pyrsistent | ||
|
||
from jsonschema.compat import str_types, int_types, iteritems | ||
from jsonschema.exceptions import UndefinedTypeCheck | ||
|
||
|
||
def is_array(instance): | ||
return isinstance(instance, list) | ||
|
||
|
||
def is_bool(instance): | ||
return isinstance(instance, bool) | ||
|
||
|
||
def is_integer(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(instance): | ||
return instance is None | ||
|
||
|
||
def is_number(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(instance): | ||
return isinstance(instance, dict) | ||
|
||
|
||
def is_string(instance): | ||
return isinstance(instance, str_types) | ||
|
||
|
||
def is_any(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 set 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 (pyrsistent.pmap): | ||
|
||
It is recommend to set type checkers through | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, is there a reason to recommend that over the param to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is kind-of an awkwardness with the attrs I suppose that we could have init take in a standard dictionary instead, which is then converted to the pmap by the Thoughts? Sorry this didn't come up earlier, I overlooked this before as the init is generated by attrs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah so this is the juggle to make it work:
As it happens, this is the same as the attrs issue you open a few months back: python-attrs/attrs#207 I think this is probably the right solution, however. It means that checkers can be defined on init without users needing to supply a Edit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's making you reluctant to ask for a (If you did have such a reservation, you can use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels like an implementation detail. To me, the I'm not really too fussy, however. (Thanks for the pointer, that's a much cleaner way of doing it - looks like it's |
||
:meth:`TypeChecker.redefine` or :meth:`TypeChecker.redefine_many` | ||
""" | ||
_type_checkers = attr.ib(default=pyrsistent.pmap({})) | ||
|
||
def is_type(self, instance, type): | ||
""" | ||
Check if the instance is of the appropriate type. | ||
|
||
Arguments: | ||
|
||
instance (any primitive type, i.e. str, number, bool): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, it's not just primitive types is it? It's any JSON-deserialized thing? |
||
|
||
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: | ||
return self._type_checkers[type](instance) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pedantry: in theory someone can add a type checker that itself raises |
||
except KeyError: | ||
raise UndefinedTypeCheck | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should include the name of the type here (and ideally add that to the test that checks this branch) |
||
|
||
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 one parameter, instance, | ||
that checks if instance is of this type. | ||
|
||
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) | ||
evolver = self._type_checkers.evolver() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this just |
||
|
||
for type_, checker in iteritems(definitions): | ||
evolver[type_] = checker | ||
|
||
return attr.evolve(self, type_checkers=evolver.persistent()) | ||
|
||
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 | ||
|
||
return attr.evolve(self, type_checkers=evolver.persistent()) | ||
|
||
|
||
draft3_type_checker = TypeChecker().redefine_many({ | ||
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") |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,7 @@ | |
) | ||
from urllib import unquote # noqa | ||
from urllib2 import urlopen # noqa | ||
str_types = basestring | ||
str_types = basestring, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't need this to be a tuple, do you? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually no, I didn't need this to change in the end - at some point it was looking like I did. Isn't it odd that str_types is a tuple in PY3 and not the PY2 though? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, mostly it's Doesn't matter too much one way or the other though. |
||
int_types = int, long | ||
iteritems = operator.methodcaller("iteritems") | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -140,6 +140,9 @@ class RefResolutionError(Exception): | |
pass | ||
|
||
|
||
class UndefinedTypeCheck(Exception): | ||
pass | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2 blank lines please. |
||
class UnknownType(Exception): | ||
def __init__(self, type, instance, schema): | ||
self.type = type | ||
|
Uh oh!
There was an error while loading. Please reload this page.
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.
Might be nice to use a sphinx deprecated directive even though we haven't been very careful with
versionchanged
either...type_checker
-> ``type_checker`` might also be nice here.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.
Hah - I nearly mentioned that in the PR. I'll double check, but the deprecated directive appears to require a version that something was deprecated in and I wasn't sure of your approach to versioning & releases.
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.
Yeah -- I think if it helps that you can assume the next version will be 2.7.0 (but lemme know if there's anything else making that harder).