Align component target naming with monitoring API #295
Conversation
github-actions
bot
added
the
part:protobuf
There was a problem hiding this comment.
Pull Request Overview
This PR refactors message type names in the dispatch API to use more descriptive and consistent naming. The changes rename internal message types to better reflect their purpose as electrical component selectors, and updates the deprecated field to use a clearer naming convention.
- Renamed message types from generic names (IdSet, CategorySet, etc.) to more descriptive names (ElectricalComponentIdSelector, ElectricalComponentCategorySelector, etc.)
- Updated the deprecated field name from
component_categoriestocomponent_categories_deprecatedto clearly indicate its deprecated status - Renamed the active field from
component_categories_typestocomponent_categoriesfor better API consistency
Comments suppressed due to low confidence (2)
proto/frequenz/api/dispatch/v1/dispatch.proto:381
- The example comment references the old field name
component_categories. This should be updated tocomponent_categorieswhich now refers to field 3 (the renamedcomponent_categories_types), or the structure of the example should be verified to match the current API.
// component_categories {
proto/frequenz/api/dispatch/v1/dispatch.proto:384
- Corrected spelling of 'cateogry' to 'category'.
// {cateogry: COMPONENT_CRYPTO_MINER}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add 'deprecated' suffix to old component_categories field and remove redundant '_types' suffix from component_categories_types field. Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
llucax
left a comment
llucax
left a comment
There was a problem hiding this comment.
Other than that, and not having a lot of context on why the name changes, LGTM.
| * `CategorySet` → `ElectricalComponentCategorySelectorDeprecated` | ||
| * `CategoryTypeSet` → `ElectricalComponentCategorySelector` | ||
| * `CategoryAndType` → `ComponentCategory` | ||
| * `component_categories` → `component_categories_deprecated` (deprecated field) |
There was a problem hiding this comment.
Why the renaming here, just to make it more explicit that it is deprecated? This will make it a breaking change in the generated code (even if the wire format stays compatible).
I guess since we are introducing many breaking changes to the generated code it is not a big deal, but I still wonder...
There was a problem hiding this comment.
I mean, all changes of this PR will break the generated code-dependent projects, unfortunately, yes.
And All changes are only for syncing/better understandability.
There was a problem hiding this comment.
I don't think we should worry much about breaking changes at this point in time. Better now than later.
There was a problem hiding this comment.
Yeah, not a big issue, we can also deal with the breaking change at other level of the python abstraction.
Still for me it sounds weird to document a field is deprecated in the field name, there is even a protobuf way to do it: int32 old_field = 6 [deprecated = true]; (not sure what the effect is on generated code though.
There was a problem hiding this comment.
Still for me it sounds weird to document a field is deprecated in the field name, there is even a protobuf way to do it: int32 old_field = 6 [deprecated = true]; (not sure what the effect is on generated code though.
Yeah agreed. We could also release a new version if required. Otherwise I'd suggest to just remove the field after all.
|
The context is this comment, not much more other than that and kyle poking me |
3 participants
Summary
Synchronize naming conventions for component target selectors with the monitoring API to maintain consistency across Frequenz APIs.
Changes
*Selectorsuffix patternElectricalComponentprefix to selector types for claritycomponent_categories_types→component_categories(remove redundant_typessuffix)component_categories→component_categories_deprecated(add explicit suffix)This aligns with the naming patterns introduced in frequenz-io/frequenz-api-monitoring-common@e26eeb0.
Related