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

New formats for bytes and bits with standards-compliant abbreviations #17

Merged
merged 3 commits into from
Feb 10, 2020

Conversation

wylieconlon
Copy link

@wylieconlon wylieconlon commented Jan 31, 2020

Instead of changing the behavior of the "b" formatter I have added four new formatters. #15

  • Does not change the b bytes formatter, which uses base 1024, even though it could be considered non-standard. The non-standard formatting is used by Elasticsearch, where 1kb=1024 bytes
  • Introduces the bd bytes decimal formatter for base-1000 bytes (kB, MB, GB)
  • Introduces the bb bytes binary formatter which displays kibi/mebi/gibi bytes per the SI standards (KiB, MiB, GiB). Despite being a standard, it is less popular and might not be the appropriate default.
  • Introduces the bitb bits binary formatter for base-1024 bits (kibit, Mibit, Gibit)
  • Introduces the bitd bits decimal formatter for base-1000 bits (kbit, Mbit, Gbit)

elastic/kibana#7543

Copy link

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Looked at code and tests and LGTM. I think there are some optimizations we could make around the code, but I would not consider those part of this PR, and also questionable if we still want to do, if we want to switch to Intl anyway.

@wylieconlon wylieconlon changed the title Introduces "bd" for bytes with kilo/mega as 1000s, "bb" for bytes with kibi/mebi as 1024s New formats for bytes and bits with standards-compliant abbreviations Feb 4, 2020
@wylieconlon wylieconlon requested a review from timroes February 4, 2020 21:32
Copy link

@timroes timroes left a comment

Choose a reason for hiding this comment

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

LGTM. @spalger would you mind a second review?

Copy link

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM

@wylieconlon wylieconlon merged commit 74e9505 into elastic:kibana-fork Feb 10, 2020
@wylieconlon wylieconlon deleted the bytes-new-options branch February 10, 2020 19:52
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