Skip to content

Allow passing a base_schema to load_config() #1103

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

Merged
merged 4 commits into from
Nov 15, 2024

Conversation

llucax
Copy link
Contributor

@llucax llucax commented Nov 15, 2024

This PR adds a new argument to load_config() to allow passing a base_schema to be used when loading the configuration. This is necessary to be able to use custom fields in the schema, like the ones provided by frequenz.quantities.

It also drops support for using marshmallow_dataclas.dataclass directly. marshmallow_dataclass.dataclass is intended to be used only when using my_dataclass.Schema to get the schema. But using this is not very convenient when using type hints as they are not well-supported by marshmallow, as the load() function can't have hints.

This is actually why load_config() exists in the first place, so we are using class_schema() instead, so we don't really need that our types are decorated with marshmallow_dataclass, we can use the built-in dataclass instead, we just need to add the appropriate metadata if we want more complex validation.

Using class_shema() is also necessary to be able to pass a base_schema, which we'll need when we want to use schemas with custom fields, like the ones provided by frequenz.quantities.

Finally, it improves load_config documentation to make it more explicit that this is just a wrapper to external libraries, so users should read their documentation in full and which functions are used exactly.

@llucax llucax requested a review from a team as a code owner November 15, 2024 12:24
@llucax llucax requested review from ela-kotulska-frequenz and removed request for a team November 15, 2024 12:24
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:config Affects the configuration management labels Nov 15, 2024
@llucax llucax added this to the v1.0.0-rc1200 milestone Nov 15, 2024
@llucax llucax self-assigned this Nov 15, 2024
@llucax llucax added the scope:breaking-change Breaking change, users will need to update their code label Nov 15, 2024
@llucax llucax enabled auto-merge November 15, 2024 12:33
Comment on lines 25 to 43
dictionary using [`marshmallow.Schema.load`][] (which in turn uses the
[`marshmallow.Schema.load`][] method to do the validation and deserialization).
Copy link
Contributor

@ela-kotulska-frequenz ela-kotulska-frequenz Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using [marshmallow.Schema.load][] (which in turn uses the [marshmallow.Schema.load]

Wrong name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the first one should be class_schema. Will update.

Make it more explicit that this is just a wrapper to external libraries
so users should read their documentation in full and which functions are
used exactly.

Signed-off-by: Leandro Lucarella <[email protected]>
`marshmallow_dataclass.dataclass` is intended to be used only when using
`my_dataclass.Schema` to get the schema. But using this is not very
convenient when using type hints as they are not well supported by
`marshmallow`, as the `load()` function can't have hints.

This is actually why `load_config()` exists in the first place, so we
are using `class_schema()` instead, so we don't really need that our
types are decorated with `marshmallow_dataclass`, we can use the
built-in `dataclass` instead, we just need to add the appropriate
metadata if we want more complex validation.

Using `class_shema()` is also necessary to be able to pass a
`base_schema`, which we'll need when we want to use schemas with
custom fields, like the ones provided by `frequenz.quantities`.

Because of this, we just drop support for
`marshmallow_dataclass.dataclass` and we'll require that built-in
dataclasses are used in the future.

Signed-off-by: Leandro Lucarella <[email protected]>
This is necessary to use custom fields when loading configurations.

Signed-off-by: Leandro Lucarella <[email protected]>
This is just the last step to be able to enforce via the type system
that only built-in dataclasses are used.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax llucax force-pushed the config-base-schema branch from 13ab089 to 1be3d2c Compare November 15, 2024 14:07
@llucax
Copy link
Contributor Author

llucax commented Nov 15, 2024

Updated.

@llucax llucax added this pull request to the merge queue Nov 15, 2024
Merged via the queue into frequenz-floss:v1.x.x with commit 4484c2b Nov 15, 2024
18 checks passed
@llucax llucax deleted the config-base-schema branch November 15, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:config Affects the configuration management part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests scope:breaking-change Breaking change, users will need to update their code
Projects
Development

Successfully merging this pull request may close these issues.

2 participants