Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add some typing to the exceptions.py module #1019

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 34 additions & 22 deletions jsonschema/exceptions.py
Expand Up @@ -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()

Expand All @@ -28,17 +32,17 @@ class _Error(Exception):
def __init__(
self,
message: str,
validator=_unset,
path=(),
cause=None,
validator: str | _utils.Unset = _unset,
Copy link
Member

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 is str. It's an internal implementation detail that it can be unset 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).

Copy link
Contributor Author

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 with str. Is it really that important that Unset doesn't leak? I found it quite easy to reason about when I first saw it and understood that validator should be str unless I'm breaking the package 馃槃

Copy link
Member

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.

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?

Copy link
Contributor Author

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:

_RaisesType = typing.Union[

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.

Copy link
Member

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.

Copy link
Contributor Author

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 of autoapi be okay? You'd need to manually rewrite the index whenever you write a new module, but I think autodoc should be able to find all relevant classes needed to create the documentation.

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these _unsets still need removing / # type: ignore'ing as per the above, I still don't want them appearing in docs (I don't want end-users knowing _unset exists at all even).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 type: ignore everywhere. If it is not part of the public API then it will be hard to "hide" it from users if it is also being used in parts that are exposed to the public API (such as the exception classes).

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 type: ignore seems to do more harm than good.

Copy link
Member

Choose a reason for hiding this comment

The 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 $ref related functional fixes out, which will likely be another few weeks, but then I can perhaps think a bit more about whether it's possible to get an improvement that I'm happier with. Just being transparent to not waste any of your time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you as well for the update!

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

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 type: ignores in our code where we know that the typing stubs of jsonschema in typeshed are incorrect and where mypy warns us that we are making typing errors.
Having the typing more directly assosicated with the package would expose some implementation details, but at least the typing would be correct and we (and mypy) can rely on that.

Just being transparent to not waste any of your time.

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,
Expand Down Expand Up @@ -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,
)
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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):
Expand All @@ -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

Expand All @@ -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)

Expand Down Expand Up @@ -188,7 +200,7 @@ class RefResolutionError(Exception):

_cause = attr.ib()

def __str__(self):
def __str__(self) -> str:
return str(self._cause)


Expand All @@ -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"


Expand Down