frequenz-floss / frequenz-api-common

Shared protobuf definitions and Python bindings for Frequenz APIs
https://frequenz-floss.github.io/frequenz-api-common/
MIT License
1 stars 12 forks source link

Rename `ComponentCategoryMetadataVariant` enum #249

Open tiyash-basu-frequenz opened 1 month ago

tiyash-basu-frequenz commented 1 month ago

What's needed?

The ComponentCategoryMetadataVariant enum is named in a very verbose manner: https://github.com/frequenz-floss/frequenz-api-common/blob/5265439c14c822c14c8a2ab87fe3e2387d90cb3b/proto/frequenz/api/common/v1/microgrid/components/components.proto#L89-L100

Primarily, it is already specified as an enum, yet we add the suffix Variant to it.

Proposed solution

Maybe we just rename it to ComponentCategoryMetadata.

Use cases

No response

Alternatives and workarounds

No response

Additional context

No response

llucax commented 1 month ago

For me it sounds a bit inconsistent to name the message Metadata but the oneof messages are not named metadata.

Also not sure if you planned to create a separate issue, but the category_type field in Component also looks off.

In https://github.com/frequenz-floss/frequenz-api-common/discussions/239 I'm proposing a solution for both issues (category_type -> category_specific_info and ComponentCategoryMetadataVariant -> CategorySpecificInfo). As I say there, I'm also fine to make it metadata instead of info, I just prefer info slightly because is shorter and more accurate IMHO, this doesn't really look like metadata.

Copying the proposal here for convenience:


For one side I feel a bit like the ComponentCategory is redundant with ComponentCategoryMetadataVariant, if we would define one message for every category, then it is enough with ComponentCategoryMetadataVariant, the oneof will tell us which category we are dealing with.

But I assume the ComponentCategory is used much more widely, so we need to keep it for other purposes, so let's keep looking for ideas assuming this.

If we forget about the category, it seems like what we have here is the category-specific characteristics from a component. What about naming ComponentCategoryMetadataVariant something like CategorySpecificInfo. For me the name MetadataVariant is super confusing, and the field name category_type is even more confusing.

This is also how it reads when accessing this info:

metadata = component.category_type.WhichOneOf("metadata")
if metadata == "battery":
    battery = component.category_type.battery

For me this looks like an enum, like it is just saying it is a battery, not that we are actually getting information about the battery.

Looking at the problem in reverse, something more intuitive could look like:

category = component.category_specific_info.WhichOneOf("category")
if category == "battery":
    battery_info = component.category_specific_info.battery
// Metadata specific to a microgrid component.
message CategorySpecificInfo {
  oneof category {
    frequenz.api.common.v1.microgrid.components.BatteryInfo battery = 1;
    frequenz.api.common.v1.microgrid.components.EvChargerInfo ev_charger = 2;
    frequenz.api.common.v1.microgrid.components.FuseInfo fuse = 3;
    frequenz.api.common.v1.microgrid.components.GridConnectionPointInfo grid = 4;
    frequenz.api.common.v1.microgrid.components.InverterInfo inverter = 5;
    frequenz.api.common.v1.microgrid.components.VoltageTransformerInfo
      voltage_transformer = 6;
  }
}

// Microgrid electrical component details.
message Component {
  // ...

  // The component category. E.g., Inverter, Battery, etc.
  ComponentCategory category = 4;

  // The metadata specific to the component category type.
  CategorySpecificInfo category_specific_info = 5;

  // ...
}

We could use something different than info, an alternative could be field (CategorySpecificFields, BatteryFields, component.category_specific.fields.battery) or even the old metadata (CategorySpecificMetadata, BatteryMetadata, component.category_specific.metadata.battery). I think I personally prefer info though, it seems more general than fields and less cryptic (or more specific) than metadata.


I also don't have strong feelings about making Battery -> BatteryInfo or BatteryMetadata or whatever, you pointed some reasons for this in the meeting, although I personally like more adding the Info to make it consistent with the category_specific_info field and CategorySpecificInfo message (again, I prefer info but ok with fields or metadata too).