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.

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

Remove dependency on googleapis #187

Merged
merged 5 commits into from
Feb 19, 2024
Merged

Conversation

llucax
Copy link
Contributor

@llucax llucax commented Feb 16, 2024

To be able to do so we create a new Decimal message that is compatible with Google's Decimal message.

Fixes #152.

@llucax llucax requested a review from a team as a code owner February 16, 2024 12:31
@llucax llucax requested a review from sandovalrr February 16, 2024 12:31
@llucax llucax self-assigned this Feb 16, 2024
@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 part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) labels Feb 16, 2024
@llucax llucax added type:enhancement New feature or enhancement visitble to users scope:breaking-change Breaking change, users will need to update their code labels Feb 16, 2024
cwasicki
cwasicki previously approved these changes Feb 16, 2024
@tiyash-basu-frequenz tiyash-basu-frequenz added this to the v0.6.0 milestone Feb 16, 2024
@tiyash-basu-frequenz
Copy link
Contributor

tiyash-basu-frequenz commented Feb 16, 2024

Since this would a breaking change, marking it blocked until we release milestone v0.5.5.

@tiyash-basu-frequenz tiyash-basu-frequenz added the status:blocked Other issues must be resolved before this can be worked on label Feb 16, 2024
@llucax
Copy link
Contributor Author

llucax commented Feb 19, 2024

Since this would a breaking change, marking it blocked until we release milestone v0.5.5.

We can also branch v0.5.x now and merge this to v0.x.x, but I'm fine with the approach that's the easiest for you.

@llucax
Copy link
Contributor Author

llucax commented Feb 19, 2024

What I forgot to do, since this is a breaking, is to create a v2/ directory already to avoid ending up with the dependency issue we had with 0.5.x and previous versions.

Also fixing the package name, I noticed a whole package for decimal might be too much and maybe we can make like google and have a types package, for example.

I wanted to know your opinion before I update the PR.

@tiyash-basu-frequenz
Copy link
Contributor

We can also branch v0.5.x now and merge this to v0.x.x, but I'm fine with the approach that's the easiest for you.

Thanks for being patient 🙏 I will get v0.5.5 out today. That would be easiest for me. We do not have upcoming non-breaking changes, so we can unblock this PR later today.

What I forgot to do, since this is a breaking, is to create a v2/ directory already to avoid ending up with the dependency issue we had with 0.5.x and previous versions.

I'd suggest holding on to that idea until we have a stable v1.0. I think the upcoming release v0.6.0 is going to be our stable-release candidate.

I noticed a whole package for decimal might be too much and maybe we can make like google and have a types package, for example.

I would prefer that as well. Also, we'd need to create a new directory v1/types, and put the decimal.proto file inside it. (Also, as of now, the decimal.proto file is in the v1 directory, but is in the package v1.decimal, which is not according to our standards.)

@llucax
Copy link
Contributor Author

llucax commented Feb 19, 2024

I'd suggest holding on to that idea until we have a stable v1.0. I think the upcoming release v0.6.0 is going to be our stable-release candidate.

Still, this will make it very messy to update again, we'll have to update everything at the same time or we won't be able to use python projects with mixed versions.

My learning is, we should have probably called pre-release versions v0a1/, v0a2/, ..., etc. or something like that, so we can create new directories without feeling guilty of taking v1/ as a development version.

If this is really the only breaking change in v0.6.x, I guess it shouldn't be that bad, because I guess downstream projects can still depend on googleapis on their side. The wire format didn't really change.

@llucax
Copy link
Contributor Author

llucax commented Feb 19, 2024

I would prefer that as well. Also, we'd need to create a new directory v1/types, and put the decimal.proto file inside it.

Yeah. that's exactly what I had in mind, will do that.

(Also, as of now, the decimal.proto file is in the v1 directory, but is in the package v1.decimal, which is not according to our standards.)

Ah, right, I thought I copied what location did, but I guess I got it wrong. Anyway, will put in the types package.

@llucax
Copy link
Contributor Author

llucax commented Feb 19, 2024

Updated!

@tiyash-basu-frequenz
Copy link
Contributor

Still, this will make it very messy to update again, we'll have to update everything at the same time or we won't be able to > use python projects with mixed versions.
My learning is, we should have probably called pre-release versions

Yeah, I feel so too. Missed opportunity.

If this is really the only breaking change in v0.6.x

No, there are quite a few in the milestone.

@tiyash-basu-frequenz tiyash-basu-frequenz removed the status:blocked Other issues must be resolved before this can be worked on label Feb 19, 2024
@tiyash-basu-frequenz
Copy link
Contributor

@llucax looks like a few git conflicts need resolving.
Also, this PR is technically unblocked now, but you may want to wait for rebasing until this is merged: #192

@llucax
Copy link
Contributor Author

llucax commented Feb 19, 2024

@tiyash-basu-frequenz done!

Verified

This commit was signed with the committer’s verified signature.
llucax Leandro Lucarella
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Verified

This commit was signed with the committer’s verified signature.
llucax Leandro Lucarella
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Verified

This commit was signed with the committer’s verified signature.
llucax Leandro Lucarella
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Verified

This commit was signed with the committer’s verified signature.
llucax Leandro Lucarella
The dependency on `googleapis-common-protos` / `googleapis/googleapis`
was removed, now that we have the internal `frequenz.api.common.decimal`
we don't need the dependency anymore.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Verified

This commit was signed with the committer’s verified signature.
llucax Leandro Lucarella
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
@github-actions github-actions bot added the part:python Affects the Python bindings label Feb 19, 2024
Copy link
Contributor

@tiyash-basu-frequenz tiyash-basu-frequenz left a comment

Choose a reason for hiding this comment

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

LGTM

@llucax llucax added this pull request to the merge queue Feb 19, 2024
Merged via the queue into frequenz-floss:v0.x.x with commit d8224e7 Feb 19, 2024
11 checks passed
@llucax llucax deleted the rm-googleapis branch February 19, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) 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.

Depending on googleapis/googleapis is overkill
3 participants