Setup mypy in tox -e typing and get it to pass
#892
Conversation
Codecov Report
@@ Coverage Diff @@
## main #892 +/- ##
==========================================
- Coverage 98.27% 96.90% -1.38%
==========================================
Files 20 20
Lines 3188 3259 +71
Branches 430 446 +16
==========================================
+ Hits 3133 3158 +25
- Misses 44 79 +35
- Partials 11 22 +11
Continue to review full report at Codecov.
|
sirosen
force-pushed
the
add-mypy-config
branch
3 times, most recently
from
1516dde
to
eea2f63
Compare
jsonschema/exceptions.py
Outdated
|
|
||
| import attr | ||
|
|
||
| from jsonschema import _utils | ||
|
|
||
| WEAK_MATCHES = frozenset(["anyOf", "oneOf"]) | ||
| STRONG_MATCHES = frozenset() | ||
| WEAK_MATCHES: typing.FrozenSet[str] = frozenset(["anyOf", "oneOf"]) |
There was a problem hiding this comment.
(Ignorance question, I'll have to read myself a bit more before merging) but is there a more generic type here than this? I'd like to not say this is a frozenset, in the future it may (likely will) become a pyrsistent.pset. So just something like collections.abc.Set or whatever the typing equivalent is would be a bit preferable.
There was a problem hiding this comment.
I think the best we can do is typing.Collection[str], which is the type for collections.abc.Collection.
typing.Set doesn't work, as it is the type for builtins.set, not collections.abc.Set.
Most of collections.abc is mirrored in typing, and even collections.abc.MutableSet maps to typing.MutableSet. I guess Set was skipped because of the likelihood of confusion with the builtin set type.
In py3.9+, all of the collections.abc types are usable as generics, so we'd be able to write
WEAK_MATCHES: collections.abc.Set[str] = ...and I would expect it to work.
The big alternative, which I could apply here and elsewhere, is to use from __future__ import annotations, now that py3.6 has been dropped. The future import enables several features and behaviors, but the one that is relevant here is that it allows the use of builtin types as generics in annotations. So we can do
from __future__ import annotations
WEAK_MATCHES: frozenset[str] = ...If you prefer the results of the future import, I can apply it to all of the places where I had to add annotations.
There was a problem hiding this comment.
Yeah let's go with the future import! After that I think I'll merge. Thanks again for the help here!
There was a problem hiding this comment.
I've just gotten around to doing this. Wherever it simplified things, I added a from __future__ import annotations.
Let me know if there are any issues with it, but I think we're good to go on this! 😄
jsonschema/tests/test_validators.py
Outdated
| @@ -1662,7 +1663,7 @@ def test_False_is_not_a_schema_even_if_you_forget_to_check(self): | |||
|
|
|||
| class TestDraft3Validator(AntiDraft6LeakMixin, ValidatorTestMixin, TestCase): | |||
| Validator = validators.Draft3Validator | |||
| valid = {}, {} | |||
| valid: typing.Tuple[dict, dict] = ({}, {}) | |||
There was a problem hiding this comment.
I take it mypy complains if you don't add these types here? (If not I don't see much value in adding them here for typing the test case fixtures, the real type is in fact the type of schemas, i.e. Union[dict, bool] right now, but possibly some larger union type later, but if it's simpler to add than ignore, then yeah leave them).
There was a problem hiding this comment.
Yes, unfortunately. mypy generally complains when it sees unannotated empty dict assignments.
Adding this annotation seemed the simplest way to pacify it.
We could also add # typing: ignore comments. For tests I think it would be appropriate (I am against the use of ignore comments in most cases, since it's just as verbose as an annotation and leaves the type checker with less information).
If we don't want type checking on tests at all, we can add exclude = test_* to the mypy config, and it will exclude all test_* module names. I could do that and revert any changes to test modules, if that is preferable.
sirosen
force-pushed
the
add-mypy-config
branch
from
2e18b46
to
6660892
Compare
|
I'm going to have to make further tweaks. I forgot about how the future import behaves. Sorry for the noise; hopefully I'll have it sorted out shortly. |
sirosen
force-pushed
the
add-mypy-config
branch
from
6660892
to
215746f
Compare
This is the smallest possible change to get mypy passing on the
jsonschema codebase. The goal of this configuration is to enforce type
annotations anywhere that they appear.
That is, if a method is added to the codebase,
def foo(x: int) -> str:
return str(x)
then usages of `foo` will by type checked. If no annotations are
added, `mypy` will not type check functions.
For the most part, this keeps the impact low. The one exceptional case
is the use of `pyrsistent.pmap` as an argument to
`attr.ib(converter=...)`. Unfortunately, it causes `mypy` to
incorrectly deduce the type of the init parameter created by attrs. We
need to "explain the type of init" to mypy by creating a callable with
a concrete type to act as the converter. The callable in question
simply wraps `pmap` with a cast and presents the desired type
information to mypy.
mypy can handle `if sys.version_info` checking better than `try ... except ImportError`. This is somewhat disappointing, but the rewrite of these import lines isn't that bad and aligns with the recommendations of the mypy docs.
In pypy3, the following line is invalid:
x: Tuple[dict, dict] = {}, {}
but this variant *is* valid:
x: Tuple[dict, dict] = ({}, {})
Apply this correction where it occurs in test_validators.py
Use of the `__future__` import of annotations allows several niceties, in particular: - parametrization of builtin types as generics - `|` syntax for unions (including `| None` for optionals) Update to use the future import wherever it improves or simplifies annotations. Avoid using new typing features outside of annotations (e.g. in assignment), which fails on older pythons. This is an unfortunate wart in the way that the future import works.
Nitpick was failing on
TYPE_CHECKER: ClassVar[TypeChecker]
It's not clear what to do about this. `TypeChecker` was imported
correctly from `jsonschema._types` and is noted in the doc as
`jsonschema.TypeChecker`. We can't import the `jsonschema` name at
runtime because that would be circular.
To resolve, use `typing.TYPE_CHECKING` to conditionally import
`jsonschema` at type-checking time. This avoids the circular
import but allows us to write
TYPE_CHECKER: ClassVar[jsonschema.TypeChecker]
As a result, Sphinx correctly builds a cross-reference from the
annotation, the annotation is still accurate, and runtime behavior
is left untouched.
sirosen
force-pushed
the
add-mypy-config
branch
from
215746f
to
631fba1
Compare
Type annotations can use variable names in order to support easily named constructs (e.g. `FooType = Union[int, str]; x: FooType`). However, such variable names are then seen by sphinx autodoc, which does not evaluate them. As a result, for such a name to avoid tripping nitpick warnings, these names need to be resolvable. The simplest resolution is to remove the use of any internal variables for this purpose (at least where they would be seen by sphinx), and use the longhand description of types in such cases.
`_format.py` had a few internal variables to make type annotations shorter to write. However, these can cause issues with sphinx nitpick. Even though the variables removed in this commit aren't causing a build failure, removing them is a good precaution.
These blocks only exist to create code which evaluates under mypy but does not run at runtime. As a result, they are impossible to cover with `coverage` on a testsuite, and make sense to exclude from coverage reporting.
sirosen
force-pushed
the
add-mypy-config
branch
from
b7d9a24
to
811bab2
Compare
|
There were a few issues with the doc build which I think I have sorted out. I did some amends and rebases to try to keep a useful commit history. 631fba1 deals with issues arising from sphinx trying to build a cross-reference for 17384e7 handles a related but different case (which was not flagged in my local builds because I was running tox under py3.8, and for whatever reason it only shows up on 3.9+). If you write cdc2807 does the same kind of cleanup as 17384e7, but in a location where sphinx was not seeing the variable names. It's purely precautionary. 811bab2 omits type-checking-only code from coverage reporting, to try to get the coverage check to pass. It doesn't seem to have been sufficient, but I think it still makes sense to include. As I write this, the build is still going and the coverage check is still red, but I expect all of it to pass. |
|
Awesome! Don't worry too much about the coverage check, I'll have a look. Appreciated. |
Julian
force-pushed
the
add-mypy-config
branch
from
138d50d
to
bc4f2d5
Compare
|
Odd (and worrying) that things pass locally in Sphinx even the way you originally had it, and on the same versions of everything via tox. But I don't have time to diagnose why at the minute... Going to merge. Thanks again for the PR! |
I agree. I'm generally a big fan of the new typing stuff, but this branch seemed to run into almost all of my least-favorite things -- typing-time code in Please feel free to call me in to help out if any of this creates problems down the line. And thanks for being receptive to these changes! |
|
I very well may take you up on that offer, thanks! |
This is the smallest possible change to get mypy passing on the jsonschema codebase. The goal of this configuration is to enforce type annotations anywhere that they appear.
That is, if a method is added to the codebase,
then usages of
foowill by type checked. If no annotations are added,mypywill not type check functions.For the most part, this keeps the impact low. The one exceptional case is the use of
pyrsistent.pmapas an argument toattr.ib(converter=...). Unfortunately, it causesmypyto incorrectly deduce the type of the init parameter created by attrs. We need to "explain the type of init" to mypy by creating a callable with a concrete type to act as the converter. The callable in question simply wrapspmapwith a cast and presents the desired typeinformation to mypy.
A few asides:
I see some whitespace edits crept in when I
blacked files. I'll back those out when I can work on this more next week.I personally like
import typing as tand then usingt.Optional,t.Dict, etc. throughout a codebase. But there are various opinions on this, so I've gone withimport typingandtyping.Optional, etc. as what I believe is the least controversial choice.tox -e typingseemed consistent withtox -e style: name the env by the thing being checked, not the tool used to check it.tox -e mypywould also work.Type checking enforcement is somewhat necessary if we are to consider publishing type information from
jsonschema. I don't know if this is really desirable, but the PR shows what it would take to get type checking to pass.