added throw_on_missing flag to _get_node() #513
Conversation
|
@Jasha10, can you review? |
|
Will take a look now. |
Thanks. |
|
Yes, I've joined the Zulip chat. |
omegaconf/basecontainer.py
Outdated
| assert node is not None | ||
| assert isinstance(node, Node) |
There was a problem hiding this comment.
| assert node is not None | |
| assert isinstance(node, Node) | |
| assert isinstance(node, Node) |
No need for the first assertion, since the second one implies node is not None.
omegaconf/basecontainer.py
Outdated
| assert node is not None | ||
| assert isinstance(node, Node) |
There was a problem hiding this comment.
| assert node is not None | |
| assert isinstance(node, Node) | |
| assert isinstance(node, Node) |
omegaconf/listconfig.py
Outdated
| assert isinstance(self.__dict__["_content"], list) | ||
| if validate_access: | ||
| self._validate_get(key) | ||
| return self.__dict__["_content"][key] # type: ignore | ||
| assert self.__dict__["_content"] is not None | ||
| assert not isinstance(self.__dict__["_content"], str) |
There was a problem hiding this comment.
I am confused by these three assertions:
isinstance(_content, list)_content is not Nonenot isinstance(_content, str)
Could these maybe be replaced with a single assertion?
There was a problem hiding this comment.
Good spot, the first one is sufficient.
omegaconf/listconfig.py
Outdated
| assert self.__dict__["_content"] is not None | ||
| assert not isinstance(self.__dict__["_content"], str) |
There was a problem hiding this comment.
| assert self.__dict__["_content"] is not None | |
| assert not isinstance(self.__dict__["_content"], str) |
On my computer, the tests + mypy still pass if the later two assertions are deleted.
omegaconf/omegaconf.py
Outdated
| assert val is None or isinstance(val, Node) | ||
| if val is not None: | ||
| assert isinstance(val, Node) |
There was a problem hiding this comment.
| assert val is None or isinstance(val, Node) | |
| if val is not None: | |
| assert isinstance(val, Node) | |
| assert val is None or isinstance(val, Node) | |
| if val is not None: |
Technically the second assertion here is not necessary: mypy can infer that val has type Node based on the previous two lines.
There was a problem hiding this comment.
(If you type reveal_type(val) inside of the if block, mypy prints Revealed type is 'omegaconf.base.Node')
omegaconf/basecontainer.py
Outdated
| assert node is not None | ||
| assert isinstance(node, Node) |
omegaconf/basecontainer.py
Outdated
| assert node is not None | ||
| assert isinstance(node, Node) |
omegaconf/listconfig.py
Outdated
| assert isinstance(self.__dict__["_content"], list) | ||
| if validate_access: | ||
| self._validate_get(key) | ||
| return self.__dict__["_content"][key] # type: ignore | ||
| assert self.__dict__["_content"] is not None | ||
| assert not isinstance(self.__dict__["_content"], str) |
There was a problem hiding this comment.
Good spot, the first one is sufficient.
tests/test_base_config.py
Outdated
| ], | ||
| ) | ||
| def test_get_node_throw_on_missing(cfg: Any, key: Any) -> None: | ||
| with pytest.raises(MissingMandatoryValue): |
There was a problem hiding this comment.
| with pytest.raises(MissingMandatoryValue): | |
| with pytest.raises( | |
| MissingMandatoryValue, | |
| match="Missing (mandatory value|node) " + re.escape(f"'{key}'"), | |
| ): |
If you want to test the error strings :P
There was a problem hiding this comment.
Meh, I just normalized the exception message :)
Why use a toopick when you can use a BFG7000?
* added throw_on_missing flag to _get_node() * tighter typing * removed some extraneous assertions * standardized error message
I also noticed that _get_node() can operate on slices as well, in which case it does not return a Node but a list.
I changed it's return type to Any to address this.