Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor protobuf messages and files to establish a standard packaging scheme #131

Merged
merged 3 commits into from
Nov 8, 2023
Merged

Refactor protobuf messages and files to establish a standard packaging scheme #131

merged 3 commits into from
Nov 8, 2023

Conversation

tiyash-basu-frequenz
Copy link
Contributor

This change introduces the following packaging norm to the remaining protobuf files: the package for every protobuf file is it's path inside the protobuf directory. E.g., the package name for the message PaginationInfo is frequenz.api.common.v1.pagination, and the message lives in the file frequenz/api/common/v1/paginaation/pagination_info.proto.

@tiyash-basu-frequenz tiyash-basu-frequenz self-assigned this Nov 8, 2023
@tiyash-basu-frequenz tiyash-basu-frequenz requested a review from a team as a code owner November 8, 2023 12:29
@tiyash-basu-frequenz tiyash-basu-frequenz added this to the v0.5.0 milestone Nov 8, 2023
@github-actions github-actions bot added part:docs Affects the documentation part:protobuf Affects the protocol buffer definition files part:tests Affects the unit, integration and performance (benchmarks) tests labels Nov 8, 2023
@tiyash-basu-frequenz
Copy link
Contributor Author

Looking into the doc-generation error.

@github-actions github-actions bot added the part:python Affects the Python bindings label Nov 8, 2023
@tiyash-basu-frequenz
Update location.proto package name
c4b12f9 
This commit updates the package name of `location.proto` from
`frequenz.api.common.v1.location` to `frequenz.api.common.v1`.

This sets package name of this proto file to its parent directory name,
which seems to be a standard practice.

Signed-off-by: Tiyash Basu <tiyash.basu@frequenz.com>
@tiyash-basu-frequenz
Refactor pagination.proto
736198c 
This commit refactors `pagination.proto` to move the `PaginationParams`
and `PaginationInfo` messages to their own files, in a new directory
`pagination`.

This sets package name of this proto file to its parent directory name,
which seems to be a standard practice.

Signed-off-by: Tiyash Basu <tiyash.basu@frequenz.com>
@tiyash-basu-frequenz
Refactor metrics.proto file
0208871 
This commit refactors the metrics proto files to be more modular and
easier to maintain. The metrics proto files are now split into two
files: `metric_sample.proto` and `bounds.proto`. The former contains
the definition of the `MetricSample` message, while the latter contains
the definition of the `Bounds` message. The files have also been moved
into a new directory `metrics`, establishing their package name as
`frequenz.api.common.v1.metrics`.

Signed-off-by: Tiyash Basu <tiyash.basu@frequenz.com>
@tiyash-basu-frequenz
Copy link
Contributor Author

Looking into the doc-generation error.

Fixed

llucax
llucax approved these changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

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

LGTM. I'm curios though, what's the benefit of the split in multiple files?

@tiyash-basu-frequenz
Copy link
Contributor Author

I'm curios though, what's the benefit of the split in multiple files?

Nothing, from the perspective of rust or python. It is just cosmetic.
Although, I feel pagination/pagination_info.proto and pagination/pagination_params.proto displays the scope better. The same applies to bounds.proto and metric_sample.proto. This is also what how the files in the google api submodule are organized.

@tiyash-basu-frequenz tiyash-basu-frequenz added this pull request to the merge queue Nov 8, 2023
Merged via the queue into frequenz-floss:v0.x.x with commit 3ad73be Nov 8, 2023
11 checks passed
@tiyash-basu-frequenz tiyash-basu-frequenz deleted the message_package_updates branch November 8, 2023 13:15
@tiyash-basu-frequenz
Copy link
Contributor Author

tiyash-basu-frequenz commented Nov 10, 2023 •
edited
Loading

I'm curios though, what's the benefit of the split in multiple files?

Nothing, from the perspective of rust or python. It is just cosmetic. Although, I feel pagination/pagination_info.proto and pagination/pagination_params.proto displays the scope better. The same applies to bounds.proto and metric_sample.proto. This is also what how the files in the google api submodule are organized.

Evidently, I was incorrect about python (correct about rust though). In python, the file name gets treated as the package name, so it creeps into the import paths as well. I still feel that splitting up files still adds more structure to the messages. So I would still go ahead with this, to keep things moving. If needed, we can of course look into re-organizing it before making the first major release.

image

@llucax
Copy link
Contributor

llucax commented Nov 10, 2023

Personally I don't mind the extra package in Python, to be honest it already looks ugly in Python with the _pb2 suffix and all, and in general we want to abstract all this away in the SDK anyway, so this would only affect SDK developers or people wanting to access the APIs directly.

@tiyash-basu-frequenz tiyash-basu-frequenz mentioned this pull request Feb 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@llucax llucax llucax approved these changes

@thomas-nicolai-frequenz thomas-nicolai-frequenz thomas-nicolai-frequenz approved these changes

@stefan-brus-frequenz stefan-brus-frequenz stefan-brus-frequenz approved these changes

@TalweSingh TalweSingh Awaiting requested review from TalweSingh TalweSingh is a code owner automatically assigned from frequenz-floss/api-team

Assignees

@tiyash-basu-frequenz tiyash-basu-frequenz

Labels
part:docs Affects the documentation part:protobuf Affects the protocol buffer definition files part:python Affects the Python bindings part:tests Affects the unit, integration and performance (benchmarks) tests
Projects
None yet
Milestone
v0.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@tiyash-basu-frequenz @llucax @thomas-nicolai-frequenz @stefan-brus-frequenz