ImmutableValidatedObject: Support nested Mapping types #1573
Conversation
| This fixes #1570 for me. Is there anything that prevents this from being merged and released? Maybe the mypy-ratchet check? Would be great to have a release with this bug fixed! |
| value = ann(**value) | ||
| | ||
| # Support Sequence[ImmutableValidatedObject] | ||
| if isinstance(value, tuple) and not isinstance(ann, str): |
There was a problem hiding this comment.
This rejects strings, but in the new code all Sequences are matched, because
>>> isinstance("hi", collections.abc.Sequence) TrueCould you exclude strings from the match, to be sure, or perhaps explain why strings don't occur here if that's the case?
There was a problem hiding this comment.
No, not all Sequences are matched, only those explicitly annotated as such. This seems more in line with what is expected from the comments, tests and existing code, afaict.
>>> import typing >>> import collections.abc >>> class test_class: ... test_string: str ... test_tuple: typing.Tuple[test_class] ... test_list: typing.List[test_class] ... test_sequence: typing.Sequence[test_class] ... test_dict: typing.Dict[str, test_class] ... test_mapping: typing.Mapping[str, test_class] ... >>> for k, v in typing.get_type_hints(test_class).items(): ... match typing.get_origin(v): ... case collections.abc.Sequence | collections.abc.Mapping: ... print(f"{k}, {v}") ... test_sequence, typing.Sequence[__main__.test_class] test_mapping, typing.Mapping[str, __main__.test_class] nixops/util.py Outdated
| match typing.get_origin(ann): | ||
| | ||
| case collections.abc.Sequence: | ||
| new_value: Any = [] |
There was a problem hiding this comment.
| new_value: Any = [] | |
| new_value: List[Any] = [] |
or maybe this could be more specific.
If the mypy-ratchet job still fails after that, I won't complain.
There was a problem hiding this comment.
Did this and added Dict to the mapping one + renamed them to avoid variable redefinition. The ratchet still fails locally for me, though. Maybe it's time to disable it?
There was a problem hiding this comment.
Maybe it's time to disable it?
I think we should keep it around, so that regressions in type coverage don't go unnoticed, but also it's ok to override it when we don't seem to have a good way to improve the types in the change.
There was a problem hiding this comment.
Yeah, it's just not very helpful, since it doesn't actually point to where you have undeclared types. Does it even do what it's supposed to? I really can't tell. It doesn't exactly make it easier to contribute to NixOps, though.
Also, improve the handling of sequences and fix the sequence test.
roberth left a comment
roberth left a comment There was a problem hiding this comment.
Code looks ok, fixes the problem, has tests.
3 participants
Since typeguard was updated in #1569, the use of
deployment.keys(+ a few required options innixops-gce, likebootstrapImage), has been broken. This is because typeguard now inspects the type of mapping containers (Mapping[str, KeyOptions]and such) and nested such containers weren't handled byImmutableValidatedObject.This also improves the handling of nested sequences and fixes their associated test.
fixes #1570