Rename category type field in ElectricalComponent #333

tiyash-basu-frequenz · 2025-05-15T15:37:23Z

This commit renames the folowing:

ComponentCatgoryMetadataVariant to ElectricalComponentCategorySpecificInfo
ElectricalComponent.category_type to ElectricalComponent.category_specific_info

These renames make the entity names better reflect their purpose.

Copilot

Pull Request Overview

This PR renames a proto message and a field to improve clarity in the naming of electrical component metadata.

Renames the message from ElectricalComponentCategoryMetadataVariant to ElectricalComponentCategorySpecificInfo.
Renames the field from category_type to category_specific_info in ElectricalComponent.
Updates release notes to reflect these naming changes.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto	Renamed message and field for improved clarity regarding electrical component metadata.
RELEASE_NOTES.md	Updated the release notes to document the renaming of the proto message and field.

Comments suppressed due to low confidence (1)

proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto:351

Update the comment to reflect the new naming (e.g. 'The metadata specific to the electrical component category info.') to avoid confusion with the renamed field.

  // The metadata specific to the component category type.

RELEASE_NOTES.md

llucax · 2025-05-16T07:54:50Z

You sneaked the unification of metric and metric sample with sensors, which seems like a major changeworth of mentioning in the PR (arguably more relevant than a simple rename).

proto/frequenz/api/common/v1/metrics/metrics.proto

proto/frequenz/api/common/v1/microgrid/sensors/sensors.proto

tiyash-basu-frequenz · 2025-05-19T08:48:02Z

You sneaked the unification of metric and metric sample with sensors, which seems like a major changeworth of mentioning in the PR (arguably more relevant than a simple rename).

No that is a mistake. The commits should not be there. Will remove those.

Copilot

Pull Request Overview

A PR that renames a proto message and associated field in ElectricalComponent to better reflect their purpose.

Renames the message from ElectricalComponentCategoryMetadataVariant to ElectricalComponentCategorySpecificInfo and changes the oneof field name from metadata to info.
Updates the field name in ElectricalComponent from category_type to category_specific_info.
Amends the release notes to document these renames.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto	Renames the proto message and field to improve clarity.
RELEASE_NOTES.md	Updates release notes to reflect the renaming, though with a slight inconsistency in naming.

RELEASE_NOTES.md

Copilot

Pull Request Overview

This PR improves naming clarity by renaming the category-specific metadata for electrical components.

Rename the protobuf message from ElectricalComponentCategoryMetadataVariant to ElectricalComponentCategorySpecificInfo
Update the oneof and field name from category_type to category_specific_info
Reflect these renames in the release notes

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto	Renamed message and updated field name and oneof
RELEASE_NOTES.md	Updated release notes to document the renames

Comments suppressed due to low confidence (2)

proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto:321

Update this comment to reflect the new naming, e.g. 'Category-specific info for a microgrid component.'

// Metadata specific to a microgrid component.

proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto:351

Revise this comment to reference 'category-specific info' instead of 'metadata', for example: 'The category-specific info for the component.'

// The metadata specific to the component category type.

RELEASE_NOTES.md

llucax · 2025-05-20T08:22:50Z

proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto

-message ElectricalComponentCategoryMetadataVariant {
-  oneof metadata {
+message ElectricalComponentCategorySpecificInfo {
+  oneof info {


Super low importance, but while we are at this, and while researching some other oneof, I got the impression kind is pretty standard¹ mainly when a message only contains one oneof and the name becomes redundant with the message name, so maybe we can use that here too?

I also asked Gemini, and they recommend component or details.

Gemini response

Okay, this is a great, concrete example to discuss! Let's look at your ElectricalComponentCategorySpecificInfo message:

// Metadata specific to a microgrid component. message ElectricalComponentCategorySpecificInfo { oneof info { // <-- This is the oneof in question frequenz.api.common.v1.microgrid.electrical_components.Battery battery = 1; frequenz.api.common.v1.microgrid.electrical_components.EvCharger ev_charger = 2; frequenz.api.common.v1.microgrid.electrical_components.Fuse fuse = 3; frequenz.api.common.v1.microgrid.electrical_components.GridConnectionPoint grid = 4; frequenz.api.common.v1.microgrid.electrical_components.Inverter inverter = 5; frequenz.api.common.v1.microgrid.electrical_components.VoltageTransformer voltage_transformer = 6; } }

The containing message is ElectricalComponentCategorySpecificInfo. This tells us that the message holds information specific to a component category. The oneof then provides the actual data for one of those specific component types.

Let's evaluate the current name info and some alternatives:

info (current):

Access path: ...category_specific_info.info.battery

Pros: Short.

Cons: It's a bit redundant. "Electrical Component Category Specific Info's info" sounds like "the information of the specific information." While not terrible, it could be more distinct.

kind:

Access path: ...category_specific_info.kind.battery

Pros: This is a standard and widely understood convention for oneof fields that act as discriminated unions, indicating the "kind" of data present. It signifies that the specific info can be of the kind "battery," "ev_charger," etc.

Cons: It's a bit abstract.

type:

Access path: ...category_specific_info.type.battery

Pros: Similar to kind, very common. It clearly indicates that this field determines the specific type of the component information.

Cons: Also somewhat abstract.

component:

Access path: ...category_specific_info.component.battery

Pros: This is quite descriptive in your domain. The oneof is literally selecting which specific component's detailed message is being provided. The fields within the oneof are battery, ev_charger – these are types of components. So, saying category_specific_info.component clearly points to "the actual component data within this specific info structure."

Cons: The word "component" is already part of the message name and the broader context (ElectricalComponent). However, this isn't necessarily a strong con, as it reinforces the domain concept.

details:

Access path: ...category_specific_info.details.battery

Pros: The oneof holds the details for the specific category. This avoids the "info.info" redundancy and is quite intuitive. "Category-specific info's details" makes good sense.

Cons: A bit more generic than component.

data:

Access path: ...category_specific_info.data.battery

Pros: Similar to details. The oneof contains the actual data payload for the specific category. Clear and common for payload-carrying fields.

Cons: Also somewhat generic.

Recommendation:

Given your specific structure and domain:

Option A: component
This is a strong contender. The oneof is selecting from different component types. category_specific_info.component.battery reads very naturally as "the battery component data within the category-specific information." It's domain-relevant and clearly indicates the nature of the choice.

Option B: details or data
These are also good choices. category_specific_info.details.battery emphasizes that this oneof provides the specific, detailed information for the category. It elegantly avoids the redundancy of the original info name.

Option C: kind or type
These are safe, standard choices that any Protobuf developer would immediately understand. They emphasize the polymorphic nature of the ElectricalComponentCategorySpecificInfo message.

My top recommendation would be component.

// Metadata specific to a microgrid component. message ElectricalComponentCategorySpecificInfo { oneof component { // Renamed from 'info' to 'component' frequenz.api.common.v1.microgrid.electrical_components.Battery battery = 1; frequenz.api.common.v1.microgrid.electrical_components.EvCharger ev_charger = 2; // ... and so on } }

This feels the most direct and semantically aligned with what the oneof represents: the specific component type whose data is included.

Second choice would be details:

// Metadata specific to a microgrid component. message ElectricalComponentCategorySpecificInfo { oneof details { // Renamed from 'info' to 'details' frequenz.api.common.v1.microgrid.electrical_components.Battery battery = 1; frequenz.api.common.v1.microgrid.electrical_components.EvCharger ev_charger = 2; // ... and so on } }

This is also very clear and avoids any redundancy.

Ultimately, the best choice is one that is clear to your team and consistent with other oneof naming in your project. However, based on the information provided, component seems to offer the best balance of domain-specificity and clarity for this particular oneof. info is acceptable but less ideal than these alternatives.

Footnotes

For example: https://github.com/protocolbuffers/protobuf/blob/c4ec2a8c0e00f3107a3aa6186e9a1e490b5bca8b/src/google/protobuf/struct.proto#L62-L78 although when searching more in depth I actually didn't find soooo many uses of kind. ↩

okay, will change to kind.

Done. Ready for another look.

This commit renames the folowing: - `ComponentCatgoryMetadataVariant` to `ElectricalComponentCategorySpecificInfo` - `ElectricalComponent.category_type` to `ElectricalComponent.category_specific_info` These renames make the entity names better reflect their purpose. Signed-off-by: Tiyash Basu <[email protected]>

Copilot

Pull Request Overview

This PR refactors the naming for electrical component category metadata to better reflect its purpose.

Renames the message from ElectricalComponentCategoryMetadataVariant to ElectricalComponentCategorySpecificInfo.
Renames the ElectricalComponent field from category_type to category_specific_info.
Updates the release notes accordingly.

Reviewed Changes

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

File	Description
proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto	Renamed message and field to clarify component metadata.
RELEASE_NOTES.md	Updated release notes to document the renaming changes.

Comments suppressed due to low confidence (1)

proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto:381

The comment still references 'component category type'; consider updating it to 'component category specific info' to reflect the renaming.

    // The metadata specific to the component category type.

tiyash-basu-frequenz self-assigned this May 15, 2025

Copilot AI review requested due to automatic review settings May 15, 2025 15:37

tiyash-basu-frequenz requested a review from a team as a code owner May 15, 2025 15:37

tiyash-basu-frequenz requested a review from tar-viturawong-frequenz May 15, 2025 15:37

github-actions bot added part:docs Affects the documentation part:protobuf Affects the protocol buffer definition files labels May 15, 2025

Copilot AI reviewed May 15, 2025

View reviewed changes

RELEASE_NOTES.md Outdated Show resolved Hide resolved

This was linked to issues May 15, 2025

Rename Component::component_type to improve clarity #248

Closed

Rename ComponentCategoryMetadataVariant enum #249

Closed

llucax reviewed May 16, 2025

View reviewed changes

proto/frequenz/api/common/v1/metrics/metrics.proto Outdated Show resolved Hide resolved

proto/frequenz/api/common/v1/microgrid/sensors/sensors.proto Outdated Show resolved Hide resolved

proto/frequenz/api/common/v1/microgrid/sensors/sensors.proto Show resolved Hide resolved

tiyash-basu-frequenz force-pushed the rename_category_metadata branch from b408e0d to 4dfe689 Compare May 19, 2025 09:14

tiyash-basu-frequenz requested review from Copilot and llucax May 19, 2025 09:15

Copilot AI reviewed May 19, 2025

View reviewed changes

RELEASE_NOTES.md Outdated Show resolved Hide resolved

tiyash-basu-frequenz force-pushed the rename_category_metadata branch 2 times, most recently from 63837c5 to 496c781 Compare May 19, 2025 09:47

tiyash-basu-frequenz requested a review from Copilot May 19, 2025 09:48

Copilot AI reviewed May 19, 2025

View reviewed changes

RELEASE_NOTES.md Show resolved Hide resolved

tiyash-basu-frequenz force-pushed the rename_category_metadata branch 4 times, most recently from 720a860 to 453b60e Compare May 20, 2025 07:04

llucax previously approved these changes May 20, 2025

View reviewed changes

tiyash-basu-frequenz dismissed llucax’s stale review via 59a655e May 20, 2025 13:45

tiyash-basu-frequenz force-pushed the rename_category_metadata branch from 453b60e to 59a655e Compare May 20, 2025 13:45

tiyash-basu-frequenz force-pushed the rename_category_metadata branch from 59a655e to 27fedfd Compare May 20, 2025 13:46

tiyash-basu-frequenz requested a review from llucax May 20, 2025 13:46

tiyash-basu-frequenz requested a review from Copilot May 20, 2025 13:46

tiyash-basu-frequenz enabled auto-merge May 20, 2025 13:46

Copilot AI reviewed May 20, 2025

View reviewed changes

llucax approved these changes May 20, 2025

View reviewed changes

tiyash-basu-frequenz added this pull request to the merge queue May 20, 2025

Merged via the queue into frequenz-floss:v0.x.x with commit 8b6eb2e May 20, 2025
6 checks passed

tiyash-basu-frequenz deleted the rename_category_metadata branch May 20, 2025 14:25

Rename category type field in ElectricalComponent #333

Rename category type field in ElectricalComponent #333

Uh oh!

Conversation

tiyash-basu-frequenz commented May 15, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

llucax commented May 16, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tiyash-basu-frequenz commented May 19, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

llucax May 20, 2025

Choose a reason for hiding this comment

Footnotes

Uh oh!

tiyash-basu-frequenz May 20, 2025

Choose a reason for hiding this comment

Uh oh!

tiyash-basu-frequenz May 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants