Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ImmutableValidatedObject: Support nested Mapping types #1573

Merged
merged 1 commit into from
Dec 17, 2023

Conversation

talyz
Copy link
Contributor

@talyz talyz commented Nov 15, 2023

Since typeguard was updated in #1569, the use of deployment.keys (+ a few required options in nixops-gce, like bootstrapImage), 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 by ImmutableValidatedObject.

This also improves the handling of nested sequences and fixes their associated test.

fixes #1570

@pSub
Copy link
Member

pSub commented Dec 4, 2023

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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rejects strings, but in the new code all Sequences are matched, because

>>> isinstance("hi", collections.abc.Sequence)
True

Could you exclude strings from the match, to be sure, or perhaps explain why strings don't occur here if that's the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks ok, fixes the problem, has tests.

@roberth roberth merged commit 053668e into NixOS:master Dec 17, 2023
9 of 10 checks passed
@talyz talyz deleted the mapping-types branch December 18, 2023 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

typeguard exception with deployment.keys
3 participants