Relax the RegistryInitConfig type to allow Mapping #66
Conversation
Python 3.7 reached end of life 2023-06-27, and Python 3.8 reached end of life 2023-10-07. Support for these versions was already dropped in #64, but we can simplify the code by removing workarounds for library bugs in those releases.
We do not actually require the config to be a dict, and in other existing repos we already use other Mapping implementations with the `Registry`.
bcmills
force-pushed
the
registry-config-type
branch
from
752838e to
035aea7
Compare
CHANGES.md
Outdated
| Relax `RegistryInitConfig` from `Dict` to `Mapping`. (We do not actually require | ||
| the config to be a dict, and in other existing repos we already use other | ||
| Mapping implementations with the `Registry`.) |
There was a problem hiding this comment.
Suggestion: consistent monospace
| Relax `RegistryInitConfig` from `Dict` to `Mapping`. (We do not actually require | |
| the config to be a dict, and in other existing repos we already use other | |
| Mapping implementations with the `Registry`.) | |
| Relax `RegistryInitConfig` from `Dict` to `Mapping`. (We do not actually require | |
| the config to be a `Dict`, and in other existing repos we already use other | |
| `Mapping` implementations with the `Registry`.) |
There was a problem hiding this comment.
Pared down the entry instead. (I through I had already moved that sentence to the commit message! 😅)
| @@ -11,7 +11,7 @@ | |||
| T = TypeVar("T") | |||
|
|
|||
|
|
|||
| RegistryInitConfig = Dict[str, Any] | |||
| RegistryInitConfig = Mapping[str, Any] | |||
There was a problem hiding this comment.
Suggestion: I think RegistryConfigWrapper generally assumes dictionaries throughout it's documentation + method names (it has a _from_dict method). Not a big deal either way, but it may be worth updating these to reference the Mapping type.
There was a problem hiding this comment.
Updated the RegistryConfigWrapper._from_dict() comment. I think it's probably ok to be a bit sloppy about this in prose, though: conceptually it is “dict-like”.
| @@ -4,6 +4,12 @@ | |||
|
|
|||
| # Changelog | |||
|
|
|||
| ## v1.3.0 | |||
There was a problem hiding this comment.
Suggestion (Optional): Could this be a patch bump? I bet we just weren't aware of this distinction when the code was first written - making this a bug IMO. I could see this going either way. Your call!
There was a problem hiding this comment.
In theory it could be, but to me this really is a feature addition: one can now legitimately use a Mapping where only a Dict was (technically) allowed.
CHANGES.md describes the contents of the change; the commit message describes the reason for making it.
We do not actually require the config to be a dict, and in other existing repos we already use other
Mappingimplementations with theRegistry.