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

Initialize bytefmt package #1

Merged
merged 10 commits into from
Feb 4, 2021
Merged

Initialize bytefmt package #1

merged 10 commits into from
Feb 4, 2021

Conversation

ckarenz
Copy link
Contributor

@ckarenz ckarenz commented Jan 28, 2021

This package solves a number of issues we've encountered with byte quantity parsing and formatting. One or more of the following problems appear in every existing library I could find.

  1. Most packages force a particular radix (metric or binary) instead of respecting SI standards.
  2. Most packages cast everything to float and lose precision.
  3. Existing packages discard radix during parsing, making it impossible to round-trip a value as written.

This package is inspired by Kubernetes' Quantity value. In most cases it's not appropriate to use that package because it requires a dependency on Kubernetes. It's also much more awkward to use due to being a generalized solution that does not presume byte units.

Remaining work:

  • Format can lose precision, and doesn't always round-trip exactly. This is resolvable, but I want to get the library in place before focusing on the more esoteric aspects of formatting. It might be that the package is good enough as written now.
  • Write a readme.
  • Integrate CI.

}
}

func assertNoErr(t *testing.T, err error, message string, args ...interface{}) bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are here because this library is intended to be a common dependency. I don't want to pull in testify for such a small test suite.

bytefmt_test.go Outdated Show resolved Hide resolved
bytefmt.go Show resolved Hide resolved
bytefmt.go Show resolved Hide resolved
Copy link

@aimichal aimichal left a comment

Choose a reason for hiding this comment

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

Looks like a fun lib :)

I haven't used any existing libraries for this, but it'd be helpful to point out (in the docs of this package) specific cases where the established libs are problematic?

bytefmt.go Show resolved Hide resolved
bytefmt.go Outdated Show resolved Hide resolved
@ckarenz ckarenz marked this pull request as ready for review February 4, 2021 21:33
@ckarenz ckarenz enabled auto-merge (squash) February 4, 2021 21:45
@ckarenz ckarenz disabled auto-merge February 4, 2021 22:04
@ckarenz ckarenz enabled auto-merge (squash) February 4, 2021 22:08
Copy link

@aimichal aimichal left a comment

Choose a reason for hiding this comment

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

Nice interface. I like it.

Still not sure which of the tests would fail in the other libs that do similar things, so a comment there would be helpful to motivate this to someone who isn't clued in to the world of byte-formatting libs. Not as a jab to those libs, but just to be transparent about it.

This should be open-source, there's nothing proprietary or secret here.

}

// To avoid precision loss for large numbers, calculate size in big decimal.
// value = (whole * 10**len(frac) + frac) * scale / 10**len(frac)
Copy link

Choose a reason for hiding this comment

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

What's this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a summarized version of the arbitrary-precision math we're doing below. I'll throw in an example, something like the following:

value       = (whole * 10**len(frac) + frac) * scale                / 10**len(frac)
123.456 GiB = (123   * 1000          + 456 ) * (1024 * 1024 * 1024) / 1000 
            = 132559870623

func (s *Size) Add(y Size) { s.bytes += y.bytes }

// Sub subtracts size y from the current value.
func (s *Size) Sub(y Size) { s.bytes -= y.bytes }
Copy link

Choose a reason for hiding this comment

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

I think there isn't a test for this.

bytefmt.go Show resolved Hide resolved
@ckarenz ckarenz merged commit 0895f0b into main Feb 4, 2021
@ckarenz ckarenz deleted the cka/init branch February 4, 2021 22:55
// -1 if s < y
// 0 if s == y
// +1 if s > y
func (s *Size) Cmp(y Size) int {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is Cmp used anywhere in the standard library? This looks very similar to strings.Compare and bytes.Compare. I think it would be clearer to spell it out unless this is commonly used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These three-letter abbreviations look to be idiomatic in math packages (big.Int, Shopspring's decimal.Decimal and Kubernetes' resource.Quantity). My preference would be to err toward consistency with those packages since this lies within the same domain, but I'm open to a counter-argument.

bytefmt.go Show resolved Hide resolved
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.

3 participants