Add support for pathlib.Path to OmegaConf.load() and OmegaConf.save() #158
Conversation
There was a problem hiding this comment.
Thanks.
A few requests:
- Create an issue describing this (the one you linked to is talking about adding pathlib.Path as a supported type inside the config objects, not to the load and save API).
- Add a news fragment file (in news) referencing your new issue. (see this for context, OmegaConf uses the same pattern)
Note that your change will be released with OmegaConf 2.0 which is coming soon.
tests/test_serialization.py
Outdated
| @@ -37,6 +38,20 @@ def save_load_from_filename(conf: Container, resolve: bool, expected: Any) -> No | |||
| os.unlink(fp.name) | |||
|
|
|||
|
|
|||
| def save_load_from_pathlib_path(conf: Container, resolve: bool, expected: Any) -> None: | |||
There was a problem hiding this comment.
instead of duplicating the other function please parametrize it with pytest.mark.parametrize()
There was a problem hiding this comment.
Thanks @omry.
I was able to implement the other suggestions, and while I would like to keep things DRY, I'm not quite sure how to parametrise using the original save_load_from_filename. Could you please point me in the right direction?
There was a problem hiding this comment.
Made an implementation. Hopefully this is reasonably elegant :)
There was a problem hiding this comment.
It's good, and trickier than usual because you can't construct the object outside due to the temp file.
good job :).
omegaconf/omegaconf.py
Outdated
| @@ -183,7 +184,7 @@ def create( # noqa F811 | |||
| def load(file_: Union[str, IO[bytes]]) -> Union[DictConfig, ListConfig]: | |||
There was a problem hiding this comment.
Please update the typing information on save and load to reflect this change.
|
Thanks for the fix! |
|
whoops, failed lint. For next time: |
|
Oops, strange. I ran only pytest. Will run nox also next time. Thanks for the quick responses and help! |
|
cheers and thanks for the PR! |
omry
changed the title
Addresses issue #159
Test for pathlib.Path added.