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: document default length as buffer.length #40354

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

wbt
Copy link
Contributor

@wbt wbt commented Oct 6, 2021

The Buffer documentation documents the 'buf.length' property as:
'Returns the number of bytes in buf.'
It uses the .length property in its own internal references on that page.
Buffer.byteLength is a static function which takes a string parameter.
The documentation was confusing; this PR should help clarify it.

The Buffer documentation documents the 'buf.length' property as: 
'Returns the number of bytes in buf.'
It uses the .length property in its own internal references on that page.
Buffer.byteLength is a static function which takes a string parameter.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Oct 6, 2021
@wbt
Copy link
Contributor Author

wbt commented Oct 6, 2021

One character over on one line of a commit comment. Kind of frustrating to have to redo for that! Maybe someone else can pick it up from here.

@@ -3022,7 +3022,7 @@ changes:
* `options` {Object}
* `buffer` {Buffer|TypedArray|DataView} **Default:** `Buffer.alloc(16384)`
* `offset` {integer} **Default:** `0`
* `length` {integer} **Default:** `buffer.byteLength`
* `length` {integer} **Default:** `buffer.length` (`.byteLength` for TypedArray|DataView types)
Copy link
Member

Choose a reason for hiding this comment

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

I personally don't think this makes it much clearer. buffer.byteLength is correct for Buffer and TypedArray, whereas buffer.length is only correct for Buffer (and Uint8Array).

buffer.byteLength is always correct, buffer.length is not.

Copy link
Contributor Author

@wbt wbt Oct 7, 2021

Choose a reason for hiding this comment

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

According to the documentation, Buffer.byteLength is a function that does not return the number of bytes in the buffer. Buffer.length does provide that data.

Copy link
Member

Choose a reason for hiding this comment

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

@wbt Buffer extends Uint8Array. buffer.byteLength is the instance property that is inherited from Uint8Array. Buffer.byteLength() (capital B) is the static method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tniessen Does the latest commit seem to provide more helpful clarification?

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I agree with Tobias, we should keep referencing .byteLength – it's also what the code is doing.

doc/api/fs.md Show resolved Hide resolved
to meet 80 character limit
enabling easier find on page for folks coming from the fs documentation, per comment at nodejs#40354 (comment)
since this editor doesn't do that automatically....
@@ -2308,6 +2308,7 @@ added: v0.1.90
* {integer}

Returns the number of bytes in `buf`.
An alias of the inherited [`buf.byteLength`][].
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's more correct to say it's the inherited buf.length – which happens to be the same as buf.byteLength. I would remove this change from this PR.

@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label May 11, 2024
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@wbt
Copy link
Contributor Author

wbt commented May 29, 2024

I think there's still a useful contribution, but blockers like being one character over on a commit comment underscore somewhat artificially high barriers to contribution here.

@RedYetiDev
Copy link
Member

I think there's still a useful contribution, but blockers like being one character over on a commit comment underscore somewhat artificially high barriers to contribution here.

Chances are your contribution is useful :-), but if you'd like to provide information about the "artistically high barriers", please open a new issue, and im sure members of Node.js will be happy to discuss!

@wbt
Copy link
Contributor Author

wbt commented Jun 10, 2024

The PR was originally rejected because one of the six commit comments was a single character too long, and the process of trying to go through and change history is a nontrivial amount of work relative to the size of the proposed change.

If someone's interested in learning more about the large total value of contributions that is discarded when small valuable contributions are no longer worth making because the cost of making the contribution exceeds the value of the individual contribution, I'd recommend Clay Shirky's books (example here) as an entry point. While I don't think the barrier to contribution should be at zero, I don't think it should be higher than is really necessary and this rejection really felt like a strong example of the barriers being higher than necessary.

@RedYetiDev
Copy link
Member

Here are the instructions, at least how I understand them

  1. Rebase
git rebase -i HEAD~6
  1. Squash
    Replace all picks with squashes (except the first one)
  2. Commit
    Comment out all the commit messages, and insert a new one

@jakecastelli
Copy link
Member

Hi @wbt please don't feel discouraged, if you don't mind resolving the conflict, I could help you make the commit message body pass the pipeline check (as a vim user this is really just press gq to wrap the line which takes me less than a second to do), and agreed that maybe some toolings or scrips should be there for resolving this kind of frustration from contributing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants