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

Use int type for SizeInBytes in file summary response #13

Merged
merged 2 commits into from
May 24, 2022

Conversation

chrisbodhi
Copy link
Collaborator

Closes #11

Copy link

@jmoney jmoney left a comment

Choose a reason for hiding this comment

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

Use int64

Copy link

@jmoney jmoney left a comment

Choose a reason for hiding this comment

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

Bonus points if you use unint, as you can't have negative bytes

@chrisbodhi
Copy link
Collaborator Author

Using int (or uint) is more portable, because Go will use either int32 or int64 under the hood, depending on the machine's architecture. I'll change it to uint.

@chrisbodhi chrisbodhi requested a review from jmoney May 24, 2022 13:29
@@ -121,7 +121,7 @@ type FileSummaryResponse struct {
Description string `json:"description,omitempty"`
Labels []string `json:"labels,omitempty"`
Name string `json:"name"`
SizeInBytes string `json:"sizeInBytes,omitempty"`
SizeInBytes uint `json:"sizeInBytes,omitempty"`
Copy link

Choose a reason for hiding this comment

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

uint64

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jmoney Given my response above about portability, what's the upside of uint64 over uint? Also, what's our cap on dataset size? It's gotta be less than 2^64 bytes, yeah? 😁

Copy link

Choose a reason for hiding this comment

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

totally. just going with patterns. when you work in bytes typically you use 64bits. most libs accept 64bits for number of bytes and you'd do some weird conversions to get it there.

however you are right on portability, and the lib using 64bits is purely anecdotal so i rescind my request for 64 bits

jmoney
jmoney previously approved these changes May 24, 2022
@chrisbodhi
Copy link
Collaborator Author

I was too cool, and rebased instead of merging. 😑

@chrisbodhi chrisbodhi merged commit 1506223 into master May 24, 2022
@chrisbodhi chrisbodhi deleted the cb-fix-sizeinbytes-type branch May 24, 2022 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unmarshal Bug in Dataset.Retrieve
2 participants