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

implement write methods #23

Closed
wants to merge 4 commits into from

Conversation

reklatsmasters
Copy link
Contributor

Implement all writeXXX methods from Buffer prototype

for (var m in methods) {
(function(m) {
BufferList.prototype[m] = function(value) {
var buf = new Buffer(methods[m]);
Copy link
Owner

Choose a reason for hiding this comment

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

no semicolons please

@rvagg
Copy link
Owner

rvagg commented Jan 20, 2016

Nice, this was always on the roadmap but I never ended up needing it. Thoughts from anyone else? @mcollina @jcrugzz?

}

for (var m in methods) {
(function(m) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would move the function out of the for loop, but that's a stylistic thing

@mcollina
Copy link
Collaborator

I'm 👍 on that, but I would add them only if we support also the offset parameter. Otherwise we are just appending, which means we are not supporting the Buffer API.

@reklatsmasters
Copy link
Contributor Author

@mcollina I'm not sure I understand the logic of the offset parameter. If bl supported only the Buffer API, then we shouldn't just append. Which value must be equal offset to just appending (because writeInt8(1, 0) rewrites first byte)?

@mcollina
Copy link
Collaborator

@reklatsmasters that's my point. If we are calling them write*, then we should support the Buffer API. Otherwise we can call them append*, which will clarify their meaning and avoid confusion.

@jcrugzz
Copy link
Collaborator

jcrugzz commented Jan 20, 2016

I'm +1 on this. More comptibility with the Buffer API sounds good to me. I agree with @mcollina, behavior should be the same.

@mcollina
Copy link
Collaborator

Just to be clear, I'm 100% fine in adding a bunch of append* methods, which I think would possibly make more sense than writing to the beginning of the list.

We should probably support both in the long run.

@reklatsmasters
Copy link
Contributor Author

@mcollina @jcrugzz Ok, I get it. I renamed all writeXXX methods to appendXXX.

@mcollina
Copy link
Collaborator

I'm 👍 on this.

If it's ok for the others, I'd recommend you update the README as well with the new APIs.

@reklatsmasters
Copy link
Contributor Author

I think for me it would be very hard to write correct README without help in pure English 😕

@mcollina
Copy link
Collaborator

@reklatsmasters write it, we'll tell you what to fix :)

@reklatsmasters
Copy link
Contributor Author

@mcollina Sorry, i`m forgot about this PR) Is this doc correct?

The group of methods appendXXX used standard byte-writing methods of the Buffer interface to writing to the end of the internal list.

@mcollina
Copy link
Collaborator

I will write all of them down here, for new users sake.

@mcollina
Copy link
Collaborator

mcollina commented May 2, 2016

Ping, any updates on this one?

@reklatsmasters
Copy link
Contributor Author

I created my own library for this: https://www.npmjs.com/package/buffer-array.

@mcollina
Copy link
Collaborator

mcollina commented May 2, 2016

Ok, thanks anyway!

@mcollina mcollina closed this May 2, 2016
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.

4 participants