Skip to content

Merge tag 'v0.3.3' into v0.x.x #85

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

Open
wants to merge 19 commits into
base: v0.x.x
Choose a base branch
from

Conversation

llucax
Copy link
Contributor

@llucax llucax commented Jul 1, 2025

  • A new module frequenz.client.common.enum_proto has been added, which provides a generic enum_from_proto() function to convert protobuf enums to Python enums.
  • Documentation for some frequenz.client.common.microgrid.electrical_components.ElectricalComponentCategory values was improved.
  • The metrics and components enums .from_proto() are deprecated, please use the new enum_from_proto() instead.
  • Classes to represent microgrid-related ID were added (MicrogridId, EnterpriseId, ComponentId, and SensorId).

llucax and others added 18 commits June 6, 2025 12:42
Adds: EnterpriseId, MicrogridId, ComponentId and SensorId.

Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
We need to bump typing-extensions to 4.13.0 because that's the minimum
supported version declaraed by `frequenz-core`.

Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
This needs a bump of the minimum `frequenz-api-common` to v0.6.1 as this
category was added in that version.

Signed-off-by: Leandro Lucarella <[email protected]>
This function can convert any `int` to any `Enum`, with optional
validation or forward-compatibility.

Signed-off-by: Leandro Lucarella <[email protected]>
Recommend using the new `enum_from_proto()` instead.

Signed-off-by: Leandro Lucarella <[email protected]>
This is to avoid errors in tests about duplicated `BaseId` prefixes.

Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
@Copilot Copilot AI review requested due to automatic review settings July 1, 2025 09:09
@llucax llucax requested a review from a team as a code owner July 1, 2025 09:09
@llucax llucax self-assigned this Jul 1, 2025
@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:microgrid Affects the microgrid protobuf definitions labels Jul 1, 2025
@llucax llucax added cmd:skip-release-notes It is not necessary to update release notes for this PR type:enhancement New feature or enhancement visitble to users labels Jul 1, 2025
@llucax llucax enabled auto-merge July 1, 2025 09:10
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR merges tag v0.3.3 into v0.x.x, introducing a new generic enum_from_proto utility, deprecating old .from_proto() methods, and adding microgrid-related ID classes with accompanying tests.

  • Added enum_from_proto in enum_proto.py and comprehensive tests in tests/test_enum_proto.py.
  • Deprecated old .from_proto() on metrics and electrical component enums; marked for migration to enum_from_proto.
  • Introduced EnterpriseId, MicrogridId, ComponentId, and SensorId classes under microgrid, with matching tests in tests/microgrid/test_ids.py.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/test_enum_proto.py New tests to verify enum_from_proto handling of valid/invalid values
tests/microgrid/test_ids.py Tests for string/repr of new ID classes
src/frequenz/client/common/enum_proto.py New enum_from_proto implementation
src/frequenz/client/common/microgrid/init.py Added EnterpriseId and MicrogridId classes
src/frequenz/client/common/microgrid/sensors.py Added SensorId class
src/frequenz/client/common/microgrid/electrical_components/init.py Deprecated old .from_proto(), added ComponentId, enriched docs
src/frequenz/client/common/metric/init.py Deprecated old Metric.from_proto(), updated imports
pyproject.toml Bumped typing-extensions, added frequenz-core, updated warnings rules
Comments suppressed due to low confidence (2)

tests/microgrid/test_ids.py:10

  • The test imports ComponentId from microgrid.components, but the class is defined in microgrid.electrical_components. Either move ComponentId to microgrid/components.py or update the import path to frequenz.client.common.microgrid.electrical_components.
from frequenz.client.common.microgrid.components import ComponentId

src/frequenz/client/common/microgrid/electrical_components/init.py:9

  • [nitpick] The import of Enum is no longer used (classes now derive from enum.Enum). Consider removing this unused import to clean up the module.
from enum import Enum

- A new module `frequenz.client.common.enum_proto` has been added, which provides a generic `enum_from_proto()` function to convert protobuf enums to Python enums.
- Documentation for some `frequenz.client.common.microgrid.electrical_components.ElectricalComponentCategory` values was improved.
- The metrics and components enums `.from_proto()` are deprecated, please use the new `enum_from_proto()` instead.
- Classes to represent microgrid-related ID were added (`MicrogridId`, `EnterpriseId`, `ElectricalComponentId`, and `SensorId`).

* tag 'v0.3.3':
  Update release notes
  Bump the minimum version of `frequenz-core` to 1.0.2
  Deprecate uses of enum's `.from_proto()`
  Add a generic `enum_from_proto()` function
  Add the `HVAC` component category
  Add missing component categories
  Make enums unique
  Clear release notes
  Fix dependencies and prepare release notes for v0.3.2 release
  Clear release notes
  Add release notes
  Add microgrid-related ID types
  Fix warning filtering in pytest configuration
@llucax llucax disabled auto-merge July 1, 2025 09:14
@llucax
Copy link
Contributor Author

llucax commented Jul 1, 2025

Question: The ID type prefix for Electrical components before they were explicitly named "electrical" was CID. We use this prefix everywhere, so I left it as CID, even when now just CID might be ambiguous as we have non-electrical component IDs too. I guess when we add Communication Component IDs we will call them CCID and we keep the unique prefixes, but now maybe it would be too easy oversee the difference between CCID and CID so they might get confused.

So I wonder if you think we should bite the bullet and just go with ECID for *electrical component IDsand slowly update our use ofCID, or keep CID` for historical reasons?

@cwasicki @tiyash-basu-frequenz @shsms

(for more context, the prefix is used when converting a ID object to a string, for example when logging an ID)

Copy link
Contributor

@Marenz Marenz left a comment

Choose a reason for hiding this comment

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

Not quite sure how to review this 😅

@cwasicki
Copy link
Contributor

cwasicki commented Jul 1, 2025

(for more context, the prefix is used when converting a ID object to a string, for example when logging an ID)

So it mainly affects logging? Don't have objections against neither of the two options.

@llucax
Copy link
Contributor Author

llucax commented Jul 1, 2025

Yeah, it is never easy to review merges, but I would say this:

@llucax
Copy link
Contributor Author

llucax commented Jul 1, 2025

So it mainly affects logging? Don't have objections against neither of the two options.

Yeah, logging and how people refer to an electrical component ID, as if they don't match it can get pretty confusing.

@llucax
Copy link
Contributor Author

llucax commented Jul 1, 2025

Actually now that I think about it, I guess CID is not that widely used, I think MID is much more widely used (but there is no need to change that one), so maybe going for ECID is not that big of a change.

@llucax llucax added this pull request to the merge queue Jul 7, 2025
@llucax llucax removed this pull request from the merge queue due to a manual request Jul 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd:skip-release-notes It is not necessary to update release notes for this PR part:microgrid Affects the microgrid protobuf definitions part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) type:enhancement New feature or enhancement visitble to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants