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

doc: consolidate use of multiple-byte units #42587

Merged
merged 1 commit into from
Apr 19, 2022

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Apr 3, 2022

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/gyp
  • @nodejs/http
  • @nodejs/net
  • @nodejs/tsc
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 3, 2022
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 3, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2022
@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member

tniessen commented Apr 3, 2022

Similar: #35051

tniessen
tniessen previously approved these changes Apr 3, 2022
@Trott
Copy link
Member

Trott commented Apr 3, 2022

I may be behind the times on what abbreviations we can expect people to be familiar with, but I think this is going to confuse people more than enlighten them. Also FWIW the style guide doesn't seem to recommend this. https://docs.microsoft.com/en-us/style-guide/a-z-word-list-term-collections/term-collections/bits-bytes-terms

@tniessen
Copy link
Member

tniessen commented Apr 3, 2022

I think this is going to confuse people more than enlighten them

That's likely true. For memory sizes, for example, GB almost exclusively means base 2 anyway, whereas things are vastly different for data transmissions etc.

@ShogunPanda
Copy link
Contributor

I agree.
I'm not even sure that in all changed places those are actually, for instance KiB vs KB. If we want to advance this PR, we should double check just in case.

Anyway, this PR LGTM if we want to use those units.

@Trott
Copy link
Member

Trott commented Apr 3, 2022

I'm further confused by people both approving this PR and agreeing that it is likely to increase confusion.
¯\(ツ)

@ShogunPanda
Copy link
Contributor

Lol. You are definitely right.

I meant to say that the code changes look fine and we could definitely land this to improve docs(up for me on this).

But, at the same time, the possible confusion is definitely worth a second evaluation (in contrast to handle this like a trivial doc fix PR).

Hope this rant was understandable :)

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 3, 2022

My take on this is we should use the technically correct units in our code and comments, I think it's fair to assume that most folks who would read it would not be confused by them.
For our docs, I understand that confusing the reader is not something we want. Maybe a good mitigation would be to always specify the raw value next to the abbreviated unit (e.g. 16 KiB (16384 bytes)).
@Trott wdyt?

@Trott
Copy link
Member

Trott commented Apr 3, 2022

The standard that defined KiB as the abbreviation for 1024 bytes also says that the unit is a kibibyte and not a kilobyte (which is 1000 bytes, according to the standard). I don't think any of this is at all widely adopted and I'm not sure I want Node.js docs to be an early adopter for this type of thing, especially when "early adopter" means doing it after 15 years of no significant adoption of the terms.

This from 2009 still seems to be the case: [Why does Explorer use the term KB instead of KiB?}(https://devblogs.microsoft.com/oldnewthing/20090611-00/?p=17933):

If you look around you, you’ll find that nobody (to within experimental error) uses the terms kibibyte and KiB. When you buy computer memory, the amount is specified in megabytes and gigabytes, not mebibytes and gibibytes. The storage capacity printed on your blank CD is indicated in megabytes. Every document on the Internet (to within experimental error) which talks about memory and storage uses the terms kilobyte/KB, megabyte/MB, gigabyte/GB, etc. You have to go out of your way to find people who use the terms kibibyte/KiB, mebibyte/MiB, gibibyte/GiB, etc.

@Trott
Copy link
Member

Trott commented Apr 3, 2022

For our docs, I understand that confusing the reader is not something we want. Maybe a good mitigation would be to always specify the raw value next to the abbreviated unit (e.g. 16 KiB (16384 bytes)).
@Trott wdyt?

I don't love it, but it does have the virtue of precision.

Another option is to always use bytes: 16384 bytes. That will work up to a point for relatively small values, but not so much once we're talking about gigabytes or gibibytes.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@Trott
Copy link
Member

Trott commented Apr 3, 2022

(All my cranky comments are non-blocking. I just want these things considered before we land.)

@tniessen tniessen dismissed their stale review April 4, 2022 11:32

I'm further confused by people both approving this PR and agreeing that it is likely to increase confusion.

@tniessen
Copy link
Member

tniessen commented Apr 4, 2022

I'm all for favoring correctness where it matters, and especially in scientific writing, I try to consistently use IEC symbols (KiB, MiB, etc.). That being said, according to the reference @aduh95 provided, using the unit symbols KB, MB, etc. for base-2 units is not incorrect but merely legacy:

table from wikipedia

@Trott
Copy link
Member

Trott commented Apr 4, 2022

Another possibility (that might not always be practical) would be to use the common/"legacy" abbreviations but precise byte counts for clarity: 16 KB (16384 bytes)

@ShogunPanda
Copy link
Contributor

I like this one.

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 19, 2022

Given that this PR has enough approvals + wait time, I'm down to land it as it, and to improve on follow up PRs. FWIW I'd be disappointed if we had to settle on using the legacy units, but as long as we remove the confusion of when we're using metric and when we're using base-2, I'm fine with it.

@aduh95 aduh95 merged commit 1e76165 into nodejs:master Apr 19, 2022
@aduh95
Copy link
Contributor Author

aduh95 commented Apr 19, 2022

Landed in 1e76165

@aduh95 aduh95 deleted the here-comes-the-MiB branch April 19, 2022 22:46
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
Refs: https://en.wikipedia.org/wiki/Byte#Multiple-byte_units

PR-URL: nodejs#42587
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mestery <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2022
Refs: https://en.wikipedia.org/wiki/Byte#Multiple-byte_units

PR-URL: #42587
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mestery <[email protected]>
@targos targos mentioned this pull request May 2, 2022
juanarbol pushed a commit that referenced this pull request May 31, 2022
Refs: https://en.wikipedia.org/wiki/Byte#Multiple-byte_units

PR-URL: #42587
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mestery <[email protected]>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
Refs: https://en.wikipedia.org/wiki/Byte#Multiple-byte_units

PR-URL: #42587
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mestery <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
Refs: https://en.wikipedia.org/wiki/Byte#Multiple-byte_units

PR-URL: #42587
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mestery <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
Refs: https://en.wikipedia.org/wiki/Byte#Multiple-byte_units

PR-URL: #42587
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mestery <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Refs: https://en.wikipedia.org/wiki/Byte#Multiple-byte_units

PR-URL: nodejs/node#42587
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mestery <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants