Fix Structured Config assignment to Dict annotation raising wrong error. #416
Conversation
There was a problem hiding this comment.
I've noticed that it keeps modifying the dict1 value in the test cases {"foo": True} and {"user": User(age=1, name="foo")} (test_assign_wrong_type_to_dict) because when setting dict1 value to one of these it first creates a empty dict and then starts assigning its values if it passes validate_set.
class DictSubclass:
dict1: Dict[str, int] = ...
cfg.OmegaConf.structured(DictSubclass)
cfg.dict1 = {"foo": True}
# 1. {"foo": True} valid assignment so it creates {}
# 2. tries to assign True to {} but it's not a valid value_type so, it raises error
# 3. {} stays as new dict1 valueSo if validate_set encounters an invalid assignment it should be able to recover somehow with a cache for example.
| @@ -597,9 +584,9 @@ def _set_value(self, value: Any, flags: Optional[Dict[str, bool]] = None) -> Non | |||
| elif isinstance(value, dict): | |||
| for k, v in value.items(): | |||
| self.__setitem__(k, v) | |||
| else: | |||
| else: # pragma: no cover | |||
There was a problem hiding this comment.
No reason, I just moved this comment from the sentence below there because it made more sense.
There was a problem hiding this comment.
this comment says that test coverage is not considered this line as needed to be covered.
it's not a comment, it's a directive to the code coverage.
try to remove it and see if we still get 100% coverage.
if not, try to add a test that will make it get to 100%.
if it's impossible, please explain why.
There was a problem hiding this comment.
It's impossible, if it is a structured config the following code will check it.
if (
is_assign
and is_container_annotation(target_type)
and not is_container_annotation(value_type)
):If it isn't a structured config it will wrap a new node anyways.
There was a problem hiding this comment.
If this code never runs, why do we need it?
There was a problem hiding this comment.
We don't. Well to have a default case could be useful sometime but now, no.
There was a problem hiding this comment.
ok. I guess we can keep it.
If there is no reason, keep the pragma unchanged.
Close #410 .
With this fix check_assign_value_node is no longer needed.
Added a test in test_error to check the error message and, added test cases for test_assign_wrong_type_to_list and test_assign_wrong_type_to_dict for completion.