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 11 forks source link

Remove dependency on `googleapis` #187

Closed llucax closed 7 months ago

llucax commented 7 months ago

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

Fixes #152.

tiyash-basu-frequenz commented 7 months ago

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

llucax commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 :pray: 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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

Updated!

tiyash-basu-frequenz commented 7 months ago

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 commented 7 months ago

@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: https://github.com/frequenz-floss/frequenz-api-common/pull/192

llucax commented 7 months ago

@tiyash-basu-frequenz done!