Dict int key type #454
Dict int key type #454
Conversation
We will likely want to support float and bool as well (all primitive types). As for the test you commented out, you can delete it but add tests covering the usage. |
There was a problem hiding this comment.
Thanks for working on it!
Overall looks pretty good, but I think I will want more tests.
once things are settled down, you can also update the docs and finally you can create a news fragment for this change.
Per your question, it could be a good opportunity to add book and float as well to complete the spectrum of keys to all supported primitives.
This can also be done in a followup PR if you prefer (but if we go that way I will want this completed with tests and documentation updates before we land it).
We want to test that DictConfig throws KeyValidationError if invalid key type is used. But `int` type is no longer invalid, so we test on Dict[object, str] instead of on Dict[int, str].
Based on suggested changes #454 (comment) Co-authored-by: Omry Yadan <[email protected]>
|
A surface area that should be tested is Structured Configs. A few things to add tests for: Anything else I am not thinking about? |
There was a problem hiding this comment.
A related issue with type checking of OmegaConf.create:
The following also gives mypy issues:
a = OmegaConf.create({Enum1.FOO: 123})
For simplicity, we could consider changing the signature of the OmegaConf.create overload to something like obj: Union[Dict[Any, Any], None] = None.
|
re-request review when ready. |
The changes in this commit were primarly driven by feedback from mypy. Note that the definition of `DictKeyType` has been moved from dictconfig.py to basecontainer.py due to a circular import issue.
This comment has been minimized.
This comment has been minimized.
modified: omegaconf/dictconfig.py
Changing the declaration of DictConfig from
class DictConfig(BaseContainer, MutableMapping[DictKeyType, Any]):
to
class DictConfig(BaseContainer, MutableMapping[Any, Any]):
This eliminates some mypy errors.
For example, the following code was giving mypy errors when using the
old declaration:
from omegaconf import OmegaConf
cfg = OmegaConf.create()
upd = dict(a=1)
cfg.update(upd)
The mypy error was:
error: Argument 1 to "update" of "MutableMapping" has incompatible type "Dict[str, int]"; expected "Mapping[Union[str, int, Enum], Any]"
An alternative to using `MutableMapping[Any, Any]` would have been
something like
```
KT = TypeVar("KT", bound=DictKeyType)
class DictConfig(BaseContainer, MutableMapping[KT, Any]): ...
```
but this alternative would have introduced additional complexity, such
as requiring parametrization of DictConfig instances, e.g.
`DictConfig[int]` or `DictConfig[str]`.
|
@omry I think this is ready for your signoff :) |
|
Thanks for the ping, I will take a look in a few days! |
Co-authored-by: Omry Yadan <[email protected]>
Co-authored-by: Omry Yadan <[email protected]>
|
Let me know what you guys think of that suggestion. |
|
Can you add a news fragment? |
NEWS.md
Outdated
| ## 2.0.6 (2020-12-22) | ||
|
|
||
| ### Features | ||
|
|
||
| - OmegaConf now supports `int` for dictionary key types ([#149](https://github.com/omry/omegaconf/issues/149)) | ||
|
|
There was a problem hiding this comment.
sorry for not being more specific.
Do not edit the news file.
OmegaConf is using towncrier to generate this file. You can read about the general approach in this Hydra page: https://hydra.cc/docs/development/documentation#news-entries
You should create a specific file under the news directory.
This reverts commit 9af1d0b.
A preliminary stab at implementing int key-type for DictConfig.