Repository navigation
check and fixup for mypy - #123
Conversation
Codecov Report
@@ Coverage Diff @@
## main #123 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 446 447 +1
Branches 69 69
=========================================
+ Hits 446 447 +1
Continue to review full report at Codecov.
|
| field.name: field for field in fields if not field.name.startswith("_") | ||
| } | ||
| attributes: t.Dict[ | ||
| str, t.Union[dataclasses.Field, attr.Attribute, marshmallow.fields.Field] |
There was a problem hiding this comment.
This field annotation, including both types of field, seems broad. Is that what we want?
There was a problem hiding this comment.
Mmm, like it should be turned inside out? A union of two dicts such that the keys must always be either attrs or dataclasses? More like above where it is...
fields: t.Union[t.Tuple[dataclasses.Field, ...], t.Tuple[attr.Attribute, ...]]or here as...
t.Union[
t.Dict[str, t.Union[dataclasses.Field, marshmallow.fields.Field]],
t.Dict[str, t.Union[attr.Attribute, marshmallow.fields.Field]],
]There was a problem hiding this comment.
That looks better. Though tbh I forget how this works. What's the situation where we might have either an mm field or a dc field?
There was a problem hiding this comment.
I don't know why Marshmallow leaked in there... Anyways, mypy doesn't act the way I expect. Digging into it.
https://mypy-play.net/?mypy=latest&python=3.10&gist=0756f026545af8559782e541c5351d80
import typing
these_or_those: typing.Union[typing.List[int], typing.List[str]] = []
reveal_type([x for x in these_or_those])main.py:3: error: Incompatible types in assignment (expression has type "List[<nothing>]", variable has type "Union[List[int], List[str]]")
main.py:5: note: Revealed type is "builtins.list[Union[builtins.int*, builtins.str*]]"
Found 1 error in 1 file (checked 1 source file)
There was a problem hiding this comment.
I dunno. I was directed to https://mail.python.org/archives/list/typing-sig@python.org/thread/GYVM5KEE6URE6PAH7UTK6324M7GWSFQS/ as maybe being related but it seems like one way or another we will not have this "fixed" (if it is in fact a bug).
But looking again I'm not entirely convinced this line is even needed. We reassign (almost) all of the values below in the for loop (that's probably how marshmallow.fields.Field leaked in). "Almost" because the non-init fields are left untouched. But does that really make sense putting attrs/dataclass fields in the schema anyways? It seems not. So maybe just deleting this line is the "real" response to mypy complaining. Let me know what you think. If this is bad then we should add a test to point it out.
There was a problem hiding this comment.
I remember being confused about this before. Deleting it seems like a good experiment at the least.
altendky
left a comment
There was a problem hiding this comment.
thanks for the help here
| field.name: field for field in fields if not field.name.startswith("_") | ||
| } | ||
| attributes: t.Dict[ | ||
| str, t.Union[dataclasses.Field, attr.Attribute, marshmallow.fields.Field] |
There was a problem hiding this comment.
Mmm, like it should be turned inside out? A union of two dicts such that the keys must always be either attrs or dataclasses? More like above where it is...
fields: t.Union[t.Tuple[dataclasses.Field, ...], t.Tuple[attr.Attribute, ...]]or here as...
t.Union[
t.Dict[str, t.Union[dataclasses.Field, marshmallow.fields.Field]],
t.Dict[str, t.Union[attr.Attribute, marshmallow.fields.Field]],
]|
Alrighty, I think all points have been addressed and it's ready for a final review. |
No description provided.