Add some typing to the exceptions.py module
#1019
Conversation
DanielNoord
force-pushed
the
typing/base
branch
from
b69b295
to
5e79923
Compare
Codecov ReportBase: 98.22% // Head: 98.15% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1019 +/- ##
==========================================
- Coverage 98.22% 98.15% -0.07%
==========================================
Files 20 20
Lines 3545 3526 -19
Branches 537 537
==========================================
- Hits 3482 3461 -21
- Misses 47 48 +1
- Partials 16 17 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
DanielNoord
force-pushed
the
typing/base
branch
2 times, most recently
from
f809757
to
cffc822
Compare
|
I have spent way too much time getting the documentation to build 😅
jsonschema/jsonschema/_format.py Line 15 in 46fdb98 It seems silly that this should be so difficult but whatever I tried I couldn't get it to find all references successfully... Edit: After some other failed attempts this now finally seems to pass everything on CI not only locally... Turned out to be a little bigger than I had hoped. Sorry for the git spam. |
DanielNoord
force-pushed
the
typing/base
branch
2 times, most recently
from
493f11e
to
7aa8068
Compare
There was a problem hiding this comment.
Thanks for the PR! (And sorry, yes, tools love fighting with each other at this point...)
I left some initial comments coming back from Thanksgiving break. Will have to take a closer look in a bit, so probably will do a second pass. What I left so far is mostly surface level though there are some minor type tweaks.
Definitely appreciated!
jsonschema/exceptions.py
Outdated
| WEAK_MATCHES: frozenset[str] = frozenset(["anyOf", "oneOf"]) | ||
| STRONG_MATCHES: frozenset[str] = frozenset() | ||
|
|
||
| _unset = _utils.Unset() | ||
|
|
||
|
|
||
| class _Error(Exception): | ||
|
|
||
| _word_for_schema_in_error_message: str | None = None |
There was a problem hiding this comment.
This shouldn't be None, just str, though I guess I see you're adding it to the (never-instantiated _Error). But yeah can we just leave it at str?
There was a problem hiding this comment.
I used None as for most _Errors this is indeed None.
Ideally we would create a private subclass of _Error that has this defined as str and which can then be subclassed by the actual errors that have this attribute. But since I didn't want to introduce new subclasses without approval I thought I would propose this first. Would new subclasses for the classes that use this attribute be okay?
There was a problem hiding this comment.
Hm. Can you elaborate on what you mean by most _Errors having this be none?
I could be misremembering my own library, but every "encountered" instance of _Error should be of a concrete subclass (i.e. ValidationError or SchemaError), both of which define these class attributes appropriately, so no end-user should ever see an _Error where it's anything but str. Am I missing what you mean?
There was a problem hiding this comment.
Well the __str__ implementation that uses these attributes is on _Error, even though the attribute will only ever be defined on two of its subclasses. This would always throw up any typechecker as _Error.__str__ uses attributes that can not always be accessed.
This can be avoided by either adding the attribute to _Error as being None unless they are set in the two subclasses that do, or have an inbetween subclass that uses the attributes and remove the attributes from _Error.__str__
There was a problem hiding this comment.
The entire purporse of __str__ is just to code-share between the two concrete implementations, so I'm not sure I follow what you mean by introducing a(nother in-between) subclass, it would have the same issue you're mentioning.
I definitely understand the bit about a type checker not being happy with __str__ though, but yeah as I've said elsewhere, I'm not keen on making any runtime changes to satisfy the type checker, so if there's a way to just tell it to ignore the attributes great, if not I suspect an even better solution (especially given the number of exceptions is likely to grow) is going to be turning _Error into a factory function.
There was a problem hiding this comment.
I can make an example PR for the sub classes example. However, as long as the documentation doesn't build I don't think it makes a lot of sense to pursue these changes anyway. Some help with that PR would be appreciated (but I don't want to rush you, everybody has other priorities than OSS).
jsonschema/exceptions.py
Outdated
| @@ -142,12 +159,18 @@ def _contents(self): | |||
| ) | |||
| return dict((attr, getattr(self, attr)) for attr in attrs) | |||
|
|
|||
| def _matches_type(self): | |||
| def _matches_type(self) -> bool: | |||
| if isinstance(self.schema, _utils.Unset): | |||
There was a problem hiding this comment.
I'd prefer adding type: ignores rather than any runtime things here yeah.
It's a programmer error to get here if schema is somehow unset.
jsonschema/exceptions.py
Outdated
| if any(m is _unset for m in essential_for_verbose): | ||
| if ( | ||
| any(m is _unset for m in essential_for_verbose) | ||
| or self._word_for_schema_in_error_message is None |
There was a problem hiding this comment.
(Same here, I'd rather not add this, _Error is not actually meant to be instantiated, it's just an interface, though a messy one clearly with concrete behavior in it. Some day maybe it should instead be a function.)
There was a problem hiding this comment.
See my comment above!
| path=(), | ||
| cause=None, | ||
| message: str, | ||
| validator: str | _utils.Unset = _unset, |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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.
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.
There was a problem hiding this comment.
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.
jsonschema/exceptions.py
Outdated
| schema_path=(), | ||
| parent=None, | ||
| type_checker=_unset, | ||
| instance: dict[str, Any] | _utils.Unset = _unset, |
There was a problem hiding this comment.
instance is type Any (any Python object can be validated).
jsonschema/exceptions.py
Outdated
| parent=None, | ||
| type_checker=_unset, | ||
| instance: dict[str, Any] | _utils.Unset = _unset, | ||
| schema: dict[str, Any] | _utils.Unset = _unset, |
There was a problem hiding this comment.
schema is at the minute type Mapping[str, Any] | bool.
docs/spelling-wordlist.txt
Outdated
|
|
||
| Zac | ||
| HD | ||
|
|
||
| Berman | ||
| Libera | ||
| GPL | ||
|
|
||
| # json properties | ||
| additionalProperties |
There was a problem hiding this comment.
I assume this is somehow autodoc pulling things in again? Basically none of these should really be here, because none of them are really words, they should always either be surrounded in backticks (which will disable spell-checking them) or be used in :kw:foo`` roles. But autodoc can be funny there and find these words in documents we don't control...
There was a problem hiding this comment.
I can try one more time to fix this 😄
I have an idea that might work, but I can't promise much. It is indeed autodoc and autoapi pulling more stuff in.
DanielNoord
force-pushed
the
typing/base
branch
from
477caf6
to
6a16fcd
Compare
DanielNoord
force-pushed
the
typing/base
branch
from
6a16fcd
to
7f0f5e0
Compare
DanielNoord
force-pushed
the
typing/base
branch
from
7f0f5e0
to
7fd4d38
Compare
|
Rebased on |
| validator=_unset, | ||
| path=(), | ||
| cause=None, | ||
| validator: str | _utils.Unset = _unset, |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
After getting the go-ahead in #1017 I thought I would test the waters both in size and scope of potential PRs.
Feel free to ask me to separate some of the commits into separate PRs. I thought this was quite manageable, especially if you go commit by commit.
I thought I would start with a fairly self contained module (and a module I interact with quite a lot with at work myself).
I think most of the changes are pretty straight forward. The first commit is hardest to review and for now I focused on the attributes that I was most sure about, keeping harder ones for later.
The second and third commit "fix issues". I understand that these are not actual issues and that through the dynamicness of Python these code paths actually currently work. So: what is the desired action here? Add ignores to tell
mypywe know this works? Or add someisinstancechecks and attribute definitions to make the code actually type safe. I lean towards the latter but don't want to impose my personal preference on a project I am not involved with until now. I'm more than happy to change those commits to do something differenty!Happy reviewing 😄
Edit: Sorry for all the force pushes, after a helpful comment from @ AlexWaygood in the
typeshedrepo I found one issue with my initial PR.📚 Documentation preview 📚: https://python-jsonschema--1019.org.readthedocs.build/en/1019/