Skip to content

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Aug 7, 2025

This PR introduces a new module v1alpha8 that provides all the updates/changes that the common-api introduces. It also reverts the renaming of Components to ElectricalComponents to stay backwards compatible (the rename was not yet released)

Copilot AI review requested due to automatic review settings August 7, 2025 14:11
@Marenz Marenz requested a review from a team as a code owner August 7, 2025 14:11
@github-actions github-actions bot added part:docs Affects the documentation part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:microgrid Affects the microgrid protobuf definitions part:pagination Affects the pagination protobuf definitions labels Aug 7, 2025

This comment was marked as outdated.

@Marenz Marenz enabled auto-merge August 7, 2025 14:15
@Marenz Marenz disabled auto-merge August 7, 2025 14:15
@github-actions github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label Aug 7, 2025
@Marenz Marenz force-pushed the feat/update-api-common-v0.8 branch 2 times, most recently from 58381b3 to 38cb5fe Compare August 7, 2025 16:42
This reverts commit 495d2c6.

Signed-off-by: Mathias L. Baumann <[email protected]>
@Marenz Marenz force-pushed the feat/update-api-common-v0.8 branch from 38cb5fe to 396275c Compare August 7, 2025 16:43
Copy link

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 updates the frequenz-api-common dependency to v0.8.0 and adapts the codebase to support the new v1alpha8 API version while maintaining backward compatibility with the existing API.

  • Updates dependency from a git reference to a proper version constraint (v0.8.0)
  • Introduces a new v1alpha8 module structure with updated components, pagination, and metric definitions
  • Refactors the existing components module to use the v1 API and simplified naming conventions

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pyproject.toml Updates frequenz-api-common dependency to v0.8.0
src/frequenz/client/common/v1alpha8/ Adds new v1alpha8 module with electrical components, pagination, and metrics
src/frequenz/client/common/microgrid/components/ Creates new components module with simplified naming (ComponentCategory vs ElectricalComponentCategory)
tests/test_client_common_v1alpha8.py Adds tests for the new v1alpha8 electrical components
tests/test_client_common.py Updates existing tests to use the new simplified component naming
RELEASE_NOTES.md Documents the addition of v1alpha8 module support


RELAY = PBElectricalComponentCategory.ELECTRICAL_COMPONENT_CATEGORY_RELAY
BREAKER = PBElectricalComponentCategory.ELECTRICAL_COMPONENT_CATEGORY_BREAKER
"""A relay, used for switching electrical circuits on and off."""
Copy link

Copilot AI Aug 7, 2025

Choose a reason for hiding this comment

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

The documentation comment describes this as 'A relay' but the enum value is BREAKER. The documentation should be updated to reflect that this is a breaker component.

Suggested change
"""A relay, used for switching electrical circuits on and off."""
"""A breaker, used for switching and protecting electrical circuits."""

Copilot uses AI. Check for mistakes.
cwasicki
cwasicki previously approved these changes Aug 7, 2025


@dataclass(frozen=True, kw_only=True)
class Params:
Copy link
Contributor

@shsms shsms Aug 8, 2025

Choose a reason for hiding this comment

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

better keep the original name, no? This could lead to ambiguous code otherwise.

Suggested change
class Params:
class PaginationParams:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied it from v1 (non-versioned in this repo) like that. But true, we can use this opportunity to rename it in alpha8

Introduces a new module `v1alpha8` that provides all the updates/changes
that the common-api introduces.

Signed-off-by: Mathias L. Baumann <[email protected]>
@Marenz Marenz force-pushed the feat/update-api-common-v0.8 branch from 396275c to 76dda73 Compare August 11, 2025 10:36
@Marenz Marenz enabled auto-merge August 11, 2025 10:37
@Marenz Marenz disabled auto-merge August 11, 2025 15:46
@Marenz Marenz merged commit 1a8b099 into frequenz-floss:v0.x.x Aug 11, 2025
5 checks passed
@Marenz Marenz deleted the feat/update-api-common-v0.8 branch August 11, 2025 15:46
@llucax
Copy link
Contributor

llucax commented Aug 18, 2025

I'm not convinced we should just create a new namespace here as we do with protobuf. Unlike protobuf messages, which are wire-compatible, adding more classes to python could be problematic (mainly because we use type hints, if we didn't then things would work like with protobuf). If someone imports from one namespace and then try to pass the instance to a function that expects an instance from a different namespace, then things will break.

In particular classes inheriting from BaseId WILL BREAK, as they share the same prefix, so we'll get an exception as soon as someone tries to import an Id class from 2 namespaces.

We need to give more thought to this.

@llucax
Copy link
Contributor

llucax commented Aug 18, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:docs Affects the documentation part:microgrid Affects the microgrid protobuf definitions part:pagination Affects the pagination protobuf definitions part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants