Repository navigation
Use int64 for counts/sizes by default - #32
Merged
Merged
Conversation
Until now, we've been using uint32 to represent "small", non-negative values in our protobuf schema. However, there are a couple of issues with doing this: - Multiple languages including Kotlin/Java (which we use extensively on both our main client and server) don't have unsigned integer types either in the language or in protobuf generated code, forcing us to transfer unsigned values in and out of signed integer types and perform fallible conversions when interoperating with other code. - "small" is not well-defined, and while unlikely for many fields, it is quite possible for us to outgrow a uint32 for certain APIs. int64 has the same wire encoding as uint32 while mostly resolving the first issue and clearly resolving the second, so it seems to make sense to use it instead of uint32 everywhere (with appropriate protovalidate annotations to forbid negative values). Admittedly, one downside of int64 over int32 is that it isn't the "native" collection size type on the JVM on in JavaScript, so conversions might be necessary in some places. However, int32 -> int64 conversion is lossless, TypeScript/JavaScript have bigint literals for convenience, and overall it looks like using int64 by default across the API will reduce the number of conversions we have to do in our code, though we might need slightly more in TypeScript code. The "by default" is because some fields clearly have a different integer type which is defined by some external system and so should be represented similarly in our API for better interoperability. For example, right now AppPackage.target_sdk is still int32 because that's the type defined by the Android platform.
lberrymage
added
the
buf skip breaking
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Until now, we've been using uint32 to represent "small", non-negative values in our protobuf schema. However, there are a couple of issues with doing this:
int64 has the same wire encoding as uint32 while mostly resolving the first issue and clearly resolving the second, so it seems to make sense to use it instead of uint32 everywhere (with appropriate protovalidate annotations to forbid negative values). Admittedly, one downside of int64 over int32 is that it isn't the "native" collection size type on the JVM on in JavaScript, so conversions might be necessary in some places. However, int32 -> int64 conversion is lossless, TypeScript/JavaScript have bigint literals for convenience, and overall it looks like using int64 by default across the API will reduce the number of conversions we have to do in our code, though we might need slightly more in TypeScript code.
The "by default" is because some fields clearly have a different integer type which is defined by some external system and so should be represented similarly in our API for better interoperability. For example, right now AppPackage.target_sdk is still int32 because that's the type defined by the Android platform.