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: add Buffer.from(string) to functions that use buffer pool #52801

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

Whitecx
Copy link
Contributor

@Whitecx Whitecx commented May 2, 2024

Buffer.from(string) is one of the functions that may use the pre-allocated buffer. It's mentioned in the description of Buffer.from(array), but not in Buffer.from(string), or in the two other places where functions that behave this way are listed, so this commit adds those references.

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels May 2, 2024
@jasnell
Copy link
Member

jasnell commented May 4, 2024

There appear to be a number of additional commits here that are unrelated to the doc change. Can I ask you to remove the extra commits?

@Whitecx Whitecx force-pushed the update-buffer-pool-docs branch from 7fdf253 to bf02a19 Compare May 5, 2024 20:58
@Whitecx
Copy link
Contributor Author

Whitecx commented May 5, 2024

There appear to be a number of additional commits here that are unrelated to the doc change. Can I ask you to remove the extra commits?

Sorry about that, everything should be cleaned up now.

@Whitecx
Copy link
Contributor Author

Whitecx commented May 9, 2024

@jasnell Does this change feel substantive? I wanted to make this PR when I had some time, because I'd used buffer.from(string) and was confused as to why the underlying buffer had other data in it. When I didn't see any explanation in the buffer.from(string) section of the docs, I actually went to the source code and found where the buffer pool was used when creating strings long before I noticed that this behavior was documented at the bottom of buffer.from(array) 😂.

It would've saved me some time had it been in the buffer.from(string) section of the docs, so I thought it might help others as well.

@Whitecx
Copy link
Contributor Author

Whitecx commented May 19, 2024

If anyone has capacity to review this PR, I'd greatly appreciate it! If it doesn't seem worth including, I can certainly close it 🙂

doc/api/buffer.md Outdated Show resolved Hide resolved
doc/api/buffer.md Outdated Show resolved Hide resolved
@Whitecx Whitecx force-pushed the update-buffer-pool-docs branch 2 times, most recently from 95178d7 to 18263bd Compare June 8, 2024 19:50
doc/api/buffer.md Outdated Show resolved Hide resolved
@Whitecx Whitecx force-pushed the update-buffer-pool-docs branch from 18263bd to a439e5a Compare June 8, 2024 22:52
Copy link
Member

@H4ad H4ad left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

@H4ad
Copy link
Member

H4ad commented Jun 8, 2024

Oh, sorry for asking one more thing but can you please change the commit message to use add instead of added, is just for the commit message conventions.

Buffer.from(string) is one of the functions that may use the
pre-allocated buffer. It's mentioned in the description of
Buffer.from(array), but not in Buffer.from(string), or in the two other
places where functions that behave this way are listed, so this commit
adds those references.
@Whitecx Whitecx force-pushed the update-buffer-pool-docs branch from a439e5a to 214da9e Compare June 8, 2024 23:33
@Whitecx Whitecx changed the title doc: added Buffer.from(string) to functions that use buffer pool doc: add Buffer.from(string) to functions that use buffer pool Jun 8, 2024
@Whitecx
Copy link
Contributor Author

Whitecx commented Jun 8, 2024

Oh, sorry for asking one more thing but can you please change the commit message to use add instead of added, is just for the commit message conventions.

No problem! I'll keep that in mind for future PRs 🙂

@Whitecx Whitecx requested a review from H4ad June 13, 2024 19:01
@H4ad H4ad added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 14, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 14, 2024
@nodejs-github-bot nodejs-github-bot merged commit ee8e841 into nodejs:main Jun 14, 2024
16 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in ee8e841

@H4ad
Copy link
Member

H4ad commented Jun 14, 2024

@Whitecx congratulations on your first PR, thanks for contributing!

@Whitecx
Copy link
Contributor Author

Whitecx commented Jun 14, 2024

@Whitecx congratulations on your first PR, thanks for contributing!

Thank you!! And thank you for supporting with your review 😊

targos pushed a commit that referenced this pull request Jun 20, 2024
Buffer.from(string) is one of the functions that may use the
pre-allocated buffer. It's mentioned in the description of
Buffer.from(array), but not in Buffer.from(string), or in the two other
places where functions that behave this way are listed, so this commit
adds those references.

PR-URL: #52801
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
sophoniie pushed a commit to sophoniie/node that referenced this pull request Jun 20, 2024
Buffer.from(string) is one of the functions that may use the
pre-allocated buffer. It's mentioned in the description of
Buffer.from(array), but not in Buffer.from(string), or in the two other
places where functions that behave this way are listed, so this commit
adds those references.

PR-URL: nodejs#52801
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
Buffer.from(string) is one of the functions that may use the
pre-allocated buffer. It's mentioned in the description of
Buffer.from(array), but not in Buffer.from(string), or in the two other
places where functions that behave this way are listed, so this commit
adds those references.

PR-URL: nodejs#52801
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
Buffer.from(string) is one of the functions that may use the
pre-allocated buffer. It's mentioned in the description of
Buffer.from(array), but not in Buffer.from(string), or in the two other
places where functions that behave this way are listed, so this commit
adds those references.

PR-URL: #52801
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
Buffer.from(string) is one of the functions that may use the
pre-allocated buffer. It's mentioned in the description of
Buffer.from(array), but not in Buffer.from(string), or in the two other
places where functions that behave this way are listed, so this commit
adds those references.

PR-URL: #52801
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants