enable and pass mypy --strict #138
Conversation
Codecov Report
@@ Coverage Diff @@
## main #138 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 447 468 +21
Branches 69 69
=========================================
+ Hits 447 468 +21
Continue to review full report at Codecov.
|
|
Ugh, that's a lot of stuff to look back at in one big blob... |
src/desert/__init__.py
Outdated
| marshmallow_field: marshmallow.fields.Field, | ||
| *, | ||
| default: T, | ||
| metadata: t.Optional[t.Mapping[object, object]] = None, |
There was a problem hiding this comment.
Are all these default Nones a new decision or is that propagating an old decision? Why can't they be {}?
There was a problem hiding this comment.
I got itchy about the mutable default. I thought I remembered them being passed down into other function calls, but that's not happening here, so who knows what I was looking at. I'll undo that. At the very least, it doesn't belong in this PR.
| @@ -309,18 +322,22 @@ class A: | |||
|
|
|||
| schema = desert.schema_class(A)() | |||
|
|
|||
| assert schema.loads('{"x": "1.3"}') == A(decimal.Decimal("1.3")) | |||
| assert schema.loads('{"x": "1.3"}') == A(decimal.Decimal("1.3")) # type: ignore[call-arg] | |||
There was a problem hiding this comment.
My normal thinking would be to ask for comments explaining ignore directives but maybe it's not worth the bother given we have so many.
There was a problem hiding this comment.
I often do add them. I probably was just being lazy here. The issue is that we use module and so the attrs and dataclasses mypy plugins can't tell how to modify the classes we define here. Basically, the handy dynamicness of Python is getting in the way of the static mypy checking. Any ideas how to address this instead of ignoring? I guess maybe we could hint module.dataclass to support a, b, and c anyways. I'll take stab at that. It's a hack, but maybe less silly than the ignores.
There was a problem hiding this comment.
There's a basic issue that mypy that it doesn't handle class decorator syntax outside of plugins (python/mypy#3135). I'm not sure if the change below makes it better since you have to apply the decorator without the decorator syntax, and it's a lie, but this works. Any opinions on this, or other next step ideas? At this point I would stick with the ignores. Maybe a single-point explanation in the DataclassModule class would be sufficient?
diff --git a/tests/test_make.py b/tests/test_make.py
index 8efdcdb..b8c45a0 100644
--- a/tests/test_make.py
+++ b/tests/test_make.py
@@ -18,11 +18,21 @@ import typing_extensions
import desert
+@attr.frozen
+class C:
+ # Just a workaround to handle some mypy errors due to not being able to
+ # apply the attrs and dataclass awareness through the DataclassModule
+ # parametrization. Add any additional attributes that are needed by the
+ # tests.
+ x: object = None
+ y: object = None
+
+
@attr.frozen(order=False)
class DataclassModule:
"""Implementation of a dataclass module like attr or dataclasses."""
- dataclass: t.Callable[[type], type]
+ dataclass: t.Callable[[type], t.Type[C]]
field: t.Callable[..., t.Any] = attr.ib()
fields: t.Callable[[type], object] = attr.ib()
@@ -117,13 +127,14 @@ _assert_dump_load = fixture_from_dict(
def test_simple(module: DataclassModule) -> None:
"""Load dict into a dataclass instance."""
- @module.dataclass
- class A:
+ class BareA:
x: int
+ A = module.dataclass(BareA)
+
data = desert.schema_class(A)().load(data={"x": 5})
- assert data == A(x=5) # type: ignore[call-arg]
+ assert data == A(x=5)There was a problem hiding this comment.
That workaround may be more complicated than it's worth.
|
All my comments are pretty trivial so if you'd rather skip them and just merge lemme know. |
No description provided.