Skip to content

Conversation

@llucax
Copy link
Contributor

@llucax llucax commented Dec 5, 2024

  • Make all arguments keyword-only
  • Add a name argument to LoggingConfigUpdatingActor
  • Sort arguments in LoggingConfigUpdatingActor constructor
  • Initialize parent class after all attributes were initialized
  • Add missing docstring for LogLevel
  • Remove the LoggingConfig.load method
  • Use relative imports
  • Use the dataclass decorator from dataclasses
  • Group config changes in the release notes
  • Sort __all__
  • Remove unnecessary required from metadata

@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 Dec 5, 2024
@llucax
Copy link
Contributor Author

llucax commented Dec 5, 2024

Ready for review, but based on #1126.

@llucax
Copy link
Contributor Author

llucax commented Dec 5, 2024

Also in preparation for a major revamp of the config module.

@llucax llucax added type:enhancement New feature or enhancement visitble to users type:tech-debt Improves the project without visible changes for users scope:breaking-change Breaking change, users will need to update their code labels Dec 5, 2024
@llucax llucax self-assigned this Dec 5, 2024
@llucax llucax added this to the v1.0.0-rc1500 milestone Dec 5, 2024
@llucax llucax force-pushed the logging-actor branch 2 times, most recently from 4116cf0 to 229fc95 Compare December 5, 2024 14:18
Signed-off-by: Leandro Lucarella <[email protected]>
@llucax llucax marked this pull request as ready for review December 6, 2024 08:28
@llucax llucax requested a review from a team as a code owner December 6, 2024 08:28
@llucax llucax requested review from Marenz and ela-kotulska-frequenz and removed request for a team December 6, 2024 08:28
@llucax llucax enabled auto-merge December 6, 2024 08:28
@llucax
Copy link
Contributor Author

llucax commented Dec 6, 2024

Ready for review and merging.

This is just in case the parent class calls some abstract method that is
implemented in the current class, in which case it could try to accesss
the attributes that are not initialized yet.

Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
We now have `frequenz.sdk.config.load_config()` for this.

Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
We are not using the `.Schema` attribute from marshmallow_dataclass, so
it is better to just stick to the standard `dataclass` decorator to make
thing simpler.

Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
When `dataclass` field have a default value, they are not required
already, so the `required` field in the `metadata` is unnecessary.

Signed-off-by: Leandro Lucarella <[email protected]>
Copy link
Contributor

@ela-kotulska-frequenz ela-kotulska-frequenz left a comment

Choose a reason for hiding this comment

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

LGTM just one comment in README.

RELEASE_NOTES.md Outdated
* The `load()` method was removed. Please use `frequenz.sdk.config.load_config()` instead.
* The class is now a standard `dataclass` instead of a `marshmallow_dataclass`.

- `LoggerConfig` is not a standard `dataclass` instead of a `marshmallow_dataclass`.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • LoggerConfig is now a standard dataclass instead of a marshmallow_dataclass.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can be fixed in next PR

@llucax llucax added this pull request to the merge queue Dec 9, 2024
Merged via the queue into frequenz-floss:v1.x.x with commit b0a656e Dec 9, 2024
18 checks passed
@llucax llucax deleted the logging-actor branch December 9, 2024 06:51
@llucax
Copy link
Contributor Author

llucax commented Dec 9, 2024

Oh, I was just fixing it :D

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 type:enhancement New feature or enhancement visitble to users type:tech-debt Improves the project without visible changes for users

Projects

Development

Successfully merging this pull request may close these issues.

2 participants