Deprecate OmegaConf.is_optional #700
Conversation
omegaconf/omegaconf.py
Outdated
| warnings.warn( | ||
| "`OmegaConf.is_optional()` is deprecated, see https://github.com/omry/omegaconf/issues/698", | ||
| stacklevel=2, | ||
| ) | ||
| return _is_optional(obj, key) |
There was a problem hiding this comment.
I am not sure there is a good reason behind defaulting to True.
If there isn't any, I think you can maintain the old deprecated implementation here for now and make a more strict internal _is_optional.
There was a problem hiding this comment.
I think that the defaulting to True is for the case where _is_optional(obj, key) is called and key cannot be found.
Currently, no code in the omegaconf/ directory is using this "defaulting to True" branch, as all the calls use the one-argument signature _is_optional(obj) (though some code in the tests/ directory does use the two-argument signature).
Instead of returning True in the case where key cannot be found, what do you think about raising ConfigKeyError?
There was a problem hiding this comment.
Since it's an internal function and the branch is not used now, I suggest using assertion to ensure that it's not tripping anyone.
If we ever need to do something if the key does not exist we can easily change this.
Make a pass on the usage to convince yourself it's not current;y possible for a user to hit this branch though.
There was a problem hiding this comment.
Make a pass on the usage to convince yourself it's not current;y possible for a user to hit this branch though.
Yup, it's not possible for a user to hit.
Closes #698