Fixed issue where config with * could not be filled
#53
Conversation
| field = schema.__fields__[ARGS_FIELD_ALIAS] | ||
| schema.__fields__[ARGS_FIELD_ALIAS] = copy_model_field( | ||
| field, Any | ||
| ) |
There was a problem hiding this comment.
This is a copy of a logic appearing earlier (if it is a promise). It seems like it might be possible to move it outside the 'if promise'-scope.
confection/__init__.py
Outdated
| field_set = [ | ||
| k if k != ARGS_FIELD else ARGS_FIELD_ALIAS for k in result.__fields_set__ | ||
| ] |
There was a problem hiding this comment.
If validate=False this avoid removing the "*" key.
| @@ -704,6 +705,7 @@ def copy_model_field(field: ModelField, type_: Any) -> ModelField: | |||
| default=field.default, | |||
| default_factory=field.default_factory, | |||
| required=field.required, | |||
| alias=field.alias, | |||
There was a problem hiding this comment.
This avoids dropping the previous alias for *
|
Hi @svlandeg! We have some functionality dependent on this, any hints as to when this can get reviewed? :-) Loving the package and super grateful for it, so no pressure, just curious. |
I've been meaning to get to this - I saw the downstream blocked issue. Sorry that we haven't gotten to it yet! We have a few end-of-year deadlines that we need to prioritize, but I should be able to review this one on Thursday. Promise! |
There was a problem hiding this comment.
@KennethEnevoldsen & @MartinBernstorff: thanks so much for the detailed write-ups, PR and test cases! Super appreciated, especially the cat-themed test ;-)
When I use your minimal example from #47 and follow the Registry.resolve(cfg) route, it still doesn't work - not with cfg or cfg_filled and not on main or with this PR. Is that expected? I'm confused because you noted that the simple Registry.resolve(cfg) should work (even before this PR) but it doesn't on my end.
Maybe all of that is a red herring though. This PR certainly seems to fix the issue with removing the * argument from the filled config. And it allows the Registry.fill(cfg, validate=True) statements to pass, so that all looks good. The logic you implemented with the special handling of ARGS_FIELD_ALIAS looks correct to me as well.
I'll quickly double check with Ines as well, who wrote this code originally IIRC.
Seemed like it was an ongoing theme, wanted to match the style ;)
Just double checked that and it actually seems like there is an error in the example. This one works: import catalogue
from confection import Config, registry
class Registry(registry):
registry = catalogue.create("psycop", "test_reg")
@Registry.registry.register("test_fn")
def test_fn(*args: int):
return 2
config_str = """
[test_fn]
@registry = "test_fn"
* = [1, 2]
"""
if __name__ == "__main__":
cfg = Config().from_str(text=config_str)
Registry.resolve(cfg) |
|
@KennethEnevoldsen Merging this and will release it as v0.1.5a0 for now so you can keep working and we can test it! 👍 |
fixes #47