Skip to content

Conversation

@llucax
Copy link
Contributor

@llucax llucax commented Mar 17, 2025

Introduce MicrogridId and ComponentId classes to replace plain integer IDs. These classes provide type safety and prevent accidental errors by:

  • Making it impossible to mix up microgrid and component IDs (equality comparisons between different ID types always return false).
  • Preventing accidental math operations on IDs.
  • Providing clear string representations for debugging (MID42, CID42).
  • Ensuring proper hash behavior in collections.

llucax added 2 commits March 17, 2025 12:33
Signed-off-by: Leandro Lucarella <[email protected]>
The symbol in `typing` is deprecated.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax llucax requested review from a team as code owners March 17, 2025 12:16
@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:client Affects the client code labels Mar 17, 2025
@llucax
Copy link
Contributor Author

llucax commented Mar 17, 2025

This is to minimize the changes between the v0.15 and v0.17 clients, so it is easier to maintain a v0.17 branch in the SDK, as this change is very intrusive and could make merging new changes to a branch with this pretty conflict-prone.

@llucax llucax enabled auto-merge March 17, 2025 12:18
@llucax llucax requested a review from shsms March 17, 2025 12:20
@github-actions github-actions bot added the part:docs Affects the documentation label Mar 17, 2025
@llucax llucax self-assigned this Mar 17, 2025
@llucax llucax added this to the v0.7.0 milestone Mar 17, 2025
@llucax llucax added scope:breaking-change Breaking change, users will need to update their code type:enhancement New feature or enhancement visitble to users labels Mar 17, 2025
llucax added 5 commits March 17, 2025 13:46
Introduce `MicrogridId` and `ComponentId` classes to replace plain
integer IDs. These classes provide type safety and prevent accidental
errors by:

- Making it impossible to mix up microgrid and component IDs (equality
  comparisons between different ID types always return false).
- Preventing accidental math operations on IDs.
- Providing clear string representations for debugging (MID42, CID42).
- Ensuring proper hash behavior in collections.

Signed-off-by: Leandro Lucarella <[email protected]>
Now the client uses the new strongly typed IDs for components and
microgrids.

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

@shsms shsms left a comment

Choose a reason for hiding this comment

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

Just an optional comment.

Comment on lines +29 to +32
# This is not an unidiomatic typecheck, that's an odd name for the check.
# isinstance() returns True for subclasses, which is not what we want here.
# pylint: disable-next=unidiomatic-typecheck
return type(other) is MicrogridId and self._id == other._id
Copy link
Contributor

Choose a reason for hiding this comment

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

We can still do isinstance and disable inheritance for the class using the typing.final decorator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, not sure if typing.final also performs a runtime check or not, if it doesn't, then the current code is safer as it will be checked at runtime. In practice I don't think it should be a big deal anyway, I doubt anyone would want to subclass these IDs, but in any case adding typing.final() might be nice addition to make the intention clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.python.org/3.11/library/typing.html#typing.final

There is no runtime checking of these properties

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@llucax llucax added this pull request to the merge queue Mar 24, 2025
Merged via the queue into frequenz-floss:v0.x.x with commit 8bd31f1 Mar 24, 2025
14 checks passed
@llucax llucax deleted the id branch March 24, 2025 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:client Affects the client code 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants