-
Couldn't load subscription status.
- Fork 20
Add a function to load configurations with correct type hints #1097
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| # License: MIT | ||
| # Copyright © 2024 Frequenz Energy-as-a-Service GmbH | ||
|
|
||
| """Utilities to deal with configuration.""" | ||
|
|
||
| from collections.abc import Mapping | ||
| from typing import Any, TypeVar, cast | ||
|
|
||
| from marshmallow_dataclass import class_schema | ||
|
|
||
| T = TypeVar("T") | ||
| """Type variable for configuration classes.""" | ||
|
|
||
|
|
||
| def load_config( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe load_marshmallow_config ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if it is worth adding all of that to the name of the function, after all the idea was to make the loading short and neat, and it is documented what's expected. It will be a runtime error passing anything that is not a Also, this won't be able to load using a plain Footnotes
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I will update the docs and tests to reflect this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated! |
||
| cls: type[T], | ||
| config: Mapping[str, Any], | ||
| /, | ||
| **marshmallow_load_kwargs: Any, | ||
| ) -> T: | ||
| """Load a configuration from a dictionary into an instance of a configuration class. | ||
| The configuration class is expected to be a [`dataclasses.dataclass`][], which is | ||
| used to create a [`marshmallow.Schema`][] schema to validate the configuration | ||
| dictionary. | ||
| To customize the schema derived from the configuration dataclass, you can use | ||
| [`marshmallow_dataclass.dataclass`][] to specify extra metadata. | ||
| Additional arguments can be passed to [`marshmallow.Schema.load`][] using keyword | ||
| arguments. | ||
| Args: | ||
| cls: The configuration class. | ||
| config: The configuration dictionary. | ||
| **marshmallow_load_kwargs: Additional arguments to be passed to | ||
| [`marshmallow.Schema.load`][]. | ||
| Returns: | ||
| The loaded configuration as an instance of the configuration class. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand the function is internal but IMO the documentation should include the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree it could be a good addition, but the docs say very clearly that this is just a wrapper for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, it's probably not worth it. Though I'd explicitly add it if the function is exposed to the user at some point There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is already exposed via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oops, would you mind to add it in a different PR then? 👼 I have some other concern that I'd like to mention before adding any def test_none_settings() -> None:
"""Test loading None settings."""
config: dict[str, Any] = {"key": "value"}
text: str = "text"
with pytest.raises(marshmallow.ValidationError) as exc_info:
loaded_config = load_config(SimpleConfig, None) # mypy complains incompatible type "None";
with pytest.raises(marshmallow.ValidationError) as exc_info:
loaded_config = load_config(
SimpleConfig, config.get("unknown", None)
) # mypy is happpy with `None` when `get()` is usedSince There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just for the record, we had a quick call and decided it would be impossible to document every pitfall we can have with marshmallow in this function, so we'll leave it like this. If we touch this code again, the docs can be made a bit more clear about this being just a wrapper over |
||
| """ | ||
| instance = class_schema(cls)().load(config, **marshmallow_load_kwargs) | ||
| # We need to cast because `.load()` comes from marshmallow and doesn't know which | ||
| # type is returned. | ||
| return cast(T, instance) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| # License: MIT | ||
| # Copyright © 2024 Frequenz Energy-as-a-Service GmbH | ||
|
|
||
| """Tests for the config utilities.""" | ||
|
|
||
| import dataclasses | ||
| from typing import Any | ||
|
|
||
| import marshmallow | ||
| import marshmallow_dataclass | ||
| import pytest | ||
| from pytest_mock import MockerFixture | ||
|
|
||
| from frequenz.sdk.config._util import load_config | ||
|
|
||
|
|
||
| @dataclasses.dataclass | ||
| class SimpleConfig: | ||
| """A simple configuration class for testing.""" | ||
|
|
||
| name: str | ||
| value: int | ||
|
|
||
|
|
||
| @marshmallow_dataclass.dataclass | ||
| class MmSimpleConfig: | ||
| """A simple configuration class for testing.""" | ||
|
|
||
| name: str = dataclasses.field(metadata={"validate": lambda s: s.startswith("test")}) | ||
| value: int | ||
|
|
||
|
|
||
| def test_load_config_dataclass() -> None: | ||
| """Test that load_config loads a configuration into a configuration class.""" | ||
| config: dict[str, Any] = {"name": "test", "value": 42} | ||
|
|
||
| loaded_config = load_config(SimpleConfig, config) | ||
| assert loaded_config == SimpleConfig(name="test", value=42) | ||
|
|
||
| config["name"] = "not test" | ||
| loaded_config = load_config(SimpleConfig, config) | ||
| assert loaded_config == SimpleConfig(name="not test", value=42) | ||
|
|
||
|
|
||
| def test_load_config_marshmallow_dataclass() -> None: | ||
| """Test that load_config loads a configuration into a configuration class.""" | ||
| config: dict[str, Any] = {"name": "test", "value": 42} | ||
| loaded_config = load_config(MmSimpleConfig, config) | ||
| assert loaded_config == MmSimpleConfig(name="test", value=42) | ||
|
|
||
| config["name"] = "not test" | ||
| with pytest.raises(marshmallow.ValidationError): | ||
| _ = load_config(MmSimpleConfig, config) | ||
|
|
||
|
|
||
| def test_load_config_type_hints(mocker: MockerFixture) -> None: | ||
| """Test that load_config loads a configuration into a configuration class.""" | ||
| mock_class_schema = mocker.Mock() | ||
| mock_class_schema.return_value.load.return_value = {"name": "test", "value": 42} | ||
| mocker.patch( | ||
| "frequenz.sdk.config._util.class_schema", return_value=mock_class_schema | ||
| ) | ||
| config: dict[str, Any] = {} | ||
|
|
||
| # We add the type hint to test that the return type (hint) is correct | ||
| _: MmSimpleConfig = load_config(MmSimpleConfig, config, marshmallow_arg=1) | ||
| mock_class_schema.return_value.load.assert_called_once_with( | ||
| config, marshmallow_arg=1 | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!