-
-
Notifications
You must be signed in to change notification settings - Fork 581
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
Add some typing to the exceptions.py
module
#1019
Changes from all commits
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 |
---|---|---|
|
@@ -4,16 +4,20 @@ | |
from __future__ import annotations | ||
|
||
from collections import defaultdict, deque | ||
from collections.abc import Iterable, Mapping | ||
from pprint import pformat | ||
from textwrap import dedent, indent | ||
from typing import ClassVar | ||
from typing import TYPE_CHECKING, Any, ClassVar | ||
import heapq | ||
import itertools | ||
|
||
import attr | ||
|
||
from jsonschema import _utils | ||
|
||
if TYPE_CHECKING: | ||
from jsonschema import _types | ||
|
||
WEAK_MATCHES: frozenset[str] = frozenset(["anyOf", "oneOf"]) | ||
STRONG_MATCHES: frozenset[str] = frozenset() | ||
|
||
|
@@ -28,17 +32,17 @@ class _Error(Exception): | |
def __init__( | ||
self, | ||
message: str, | ||
validator=_unset, | ||
path=(), | ||
cause=None, | ||
validator: str | _utils.Unset = _unset, | ||
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 these 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. Sorry for never getting back on this, life became busy. I don't really know how to resolve this as I think it would start to become really hard to add typing to the package itself if we were to add I'd be very happy to help add typing and refactor some code so that internal details don't get exposed, but adding incorrect typing and then adding 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. Thanks for the update and no worries on the delay, sorry for my own. To be direct, kind of like #1027 (comment) I'm not sure I'm ready to merge this as-is in that case -- I have to kind of stew on it a bit. I've hopefully said a few times before, I think for documentation and editor support that I'm quite happy to merge or add typing related fixes, but beyond the two I really honestly think it's all overall more annoyance than it helps still, so while I'm slightly sympathetic, I will always still go with the former (docs) over anything else -- I want to get some 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. Thank you as well for the update!
I'm not sure this will convince you, but note that in my company we have more use cases. We have quite a bit of
Thanks for this either way! |
||
path: Iterable[str | int] = (), | ||
cause: Exception | None = None, | ||
context=(), | ||
validator_value=_unset, | ||
instance=_unset, | ||
schema=_unset, | ||
schema_path=(), | ||
parent=None, | ||
type_checker=_unset, | ||
): | ||
instance: Any = _unset, | ||
schema: Mapping[str, Any] | bool | _utils.Unset = _unset, | ||
schema_path: Iterable[str | int] = (), | ||
parent: _Error | None = None, | ||
type_checker: _types.TypeChecker | _utils.Unset = _unset, | ||
) -> None: | ||
super(_Error, self).__init__( | ||
message, | ||
validator, | ||
|
@@ -66,10 +70,10 @@ def __init__( | |
for error in context: | ||
error.parent = self | ||
|
||
def __repr__(self): | ||
def __repr__(self) -> str: | ||
return f"<{self.__class__.__name__}: {self.message!r}>" | ||
|
||
def __str__(self): | ||
def __str__(self) -> str: | ||
essential_for_verbose = ( | ||
self.validator, self.validator_value, self.instance, self.schema, | ||
) | ||
|
@@ -99,11 +103,11 @@ def __str__(self): | |
) | ||
|
||
@classmethod | ||
def create_from(cls, other): | ||
def create_from(cls, other: _Error): | ||
return cls(**other._contents()) | ||
|
||
@property | ||
def absolute_path(self): | ||
def absolute_path(self) -> deque[str | int]: | ||
parent = self.parent | ||
if parent is None: | ||
return self.relative_path | ||
|
@@ -113,7 +117,7 @@ def absolute_path(self): | |
return path | ||
|
||
@property | ||
def absolute_schema_path(self): | ||
def absolute_schema_path(self) -> deque[str | int]: | ||
parent = self.parent | ||
if parent is None: | ||
return self.relative_schema_path | ||
|
@@ -123,7 +127,7 @@ def absolute_schema_path(self): | |
return path | ||
|
||
@property | ||
def json_path(self): | ||
def json_path(self) -> str: | ||
path = "$" | ||
for elem in self.absolute_path: | ||
if isinstance(elem, int): | ||
|
@@ -132,7 +136,11 @@ def json_path(self): | |
path += "." + elem | ||
return path | ||
|
||
def _set(self, type_checker=None, **kwargs): | ||
def _set( | ||
self, | ||
type_checker: _types.TypeChecker | None = None, | ||
**kwargs: Any, | ||
) -> None: | ||
if type_checker is not None and self._type_checker is _unset: | ||
self._type_checker = type_checker | ||
|
||
|
@@ -147,12 +155,16 @@ def _contents(self): | |
) | ||
return dict((attr, getattr(self, attr)) for attr in attrs) | ||
|
||
def _matches_type(self): | ||
def _matches_type(self) -> bool: | ||
try: | ||
expected = self.schema["type"] | ||
# We ignore this as we want to simply crash if this happens | ||
expected = self.schema["type"] # type: ignore[index] | ||
except (KeyError, TypeError): | ||
return False | ||
|
||
if isinstance(self._type_checker, _utils.Unset): | ||
return False | ||
|
||
if isinstance(expected, str): | ||
return self._type_checker.is_type(self.instance, expected) | ||
|
||
|
@@ -188,7 +200,7 @@ class RefResolutionError(Exception): | |
|
||
_cause = attr.ib() | ||
|
||
def __str__(self): | ||
def __str__(self) -> str: | ||
return str(self._cause) | ||
|
||
|
||
|
@@ -197,10 +209,10 @@ class UndefinedTypeCheck(Exception): | |
A type checker was asked to check a type it did not have registered. | ||
""" | ||
|
||
def __init__(self, type): | ||
def __init__(self, type: str) -> None: | ||
self.type = type | ||
|
||
def __str__(self): | ||
def __str__(self) -> str: | ||
return f"Type {self.type!r} is unknown to this type checker" | ||
|
||
|
||
|
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.
Apparently I don't have ReadTheDocs preview builds enabled (ugh), but I'm worried all these will show up in documentation, when they shouldn't -- the user facing type of
validator
isstr
. It's an internal implementation detail that it can beunset
for the time validation is still ongoing, but I don't want users to see that, and it's not a part of its public API. Will build the docs to see what shows up (and probably enable preview builds).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.
With this PR the typing no longer shows up as adding the typing annotations completely breaks the documentation builds.
I understand that it should be
str
but there is really no way to satisfy a type checker when you are assigning_unset
to something that is annotated withstr
. Is it really that important thatUnset
doesn't leak? I found it quite easy to reason about when I first saw it and understood thatvalidator
should bestr
unless I'm breaking the package 😄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.
Hm. As in totally disables it displaying in documentation at all?
I definitely want it showing up there, it's really the only reason I care about typing at all, though I definitely feel the tooling pain with these tools, but yeah that's really the primary motivation for typing info, so I'd be worried if that's totally disabled now?
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.
The issue is that
autoapi
doesn't understand typing aliases... So even if I get all other references to resolve correctly it will still complain about:jsonschema/jsonschema/_format.py
Line 15 in 46fdb98
This is used in a signature and based on the documentation that
autoapi
builds any reference to_RaisesType
will cause a warning. That's why in the end I disabled the typing from showing up in the docs.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.
readthedocs/sphinx-autoapi#224 it looks like is maybe the/a relevant issue -- but yeah to be upfront without trying to waste any of your time I definitely would consider not showing typing info in the docs to be a non-starter unfortunately, as I say, it's the main thing I care about :/ so it may require some thinking if indeed stuff doesn't work there before merging additional changes.
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.
That's indeed the issue. Would just using
autodoc
instead ofautoapi
be okay? You'd need to manually rewrite the index whenever you write a new module, but I thinkautodoc
should be able to find all relevant classes needed to create the documentation.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.
I'm open to the idea (it's how I used to do things before autoapi). If
sphinx-apidoc
produces the right output with minimal modifications I'm even more open to it. But yeah :) if the diff is reasonable then yes.