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

docs: fs.write() variables are not clear #7868

Closed
jorangreef opened this issue Jul 25, 2016 · 12 comments
Closed

docs: fs.write() variables are not clear #7868

jorangreef opened this issue Jul 25, 2016 · 12 comments
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.

Comments

@jorangreef
Copy link
Contributor

jorangreef commented Jul 25, 2016

The docs for fs.write() state:

offset and length determine the part of the buffer to be written.

offset is clear, but one must assume that length has the same meaning as per buffer.slice(offset, length), i.e. that length is not relative to offset, but rather an absolute index into buffer as is expected (e.g. string.slice(offset, length)). Therefore, if one wanted to write 5 bytes from offset 12 at position 1 in the file, one would use offset=12, length=12+5.

... (err, written, buffer) where written specifies how many bytes were written from buffer.

This is not clear.

I would probably guess that written is a quantity number, i.e. if the first call to fs.write() returned written=2, then I would think that a count of 2 bytes had been written. I would therefore expect that fs.write(fd, buffer, offset=12, length=17, position=1) would return written=5 if everything was written out, and less than 5 if a partial write occurred.

What actually happens is that written=17 when the write is fully written out. This is very surprising. Furthermore, if written now represents an absolute index, is it an absolute index into the source buffer or an absolute index into the target file? If it is an absolute index into the target file, is it relative to position or not? It seems that written is actually then an absolute index relative to the source buffer?

Why is buffer returned in the write callback? Is this the same buffer I passed, or a slice? The docs should also make that clear.

@jorangreef
Copy link
Contributor Author

I think it's actually the other way round, length is not an absolute index into source buffer but a quantity relative to offset. Is that right?

@mscdex mscdex added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Jul 25, 2016
@jorangreef
Copy link
Contributor Author

jorangreef commented Jul 25, 2016

The docs for fs.read() are more clear:

length is an integer specifying the number of bytes to read.

Perhaps it would be better if (for both methods) length was renamed to size to differentiate it from the typical meaning of length and to imply quantity as opposed to absolute index? And if written was renamed to bytesWritten to bring it in line with fs.read()?

@jalafel
Copy link
Contributor

jalafel commented Nov 24, 2016

Hi! Looking to pick up this issue. Seems relatively easy enough! From my understanding, there is just an ambiguity with length, wherein traditionally, one might assume that it's the length of a buffer beginning from the offset index. Whereas, it actually denotes the length of the number of bytes to read.

It'd make sense to change the word choice to size, then, and expand briefly on the variable for the fs.write() documentation.

(Also changing written to bytesWritten for consistency with the documentation on fs.read().)

Let me know if there is any misunderstanding on my part!

@jorangreef
Copy link
Contributor Author

Thanks for taking it up @jessicaquynh!

@sam-github
Copy link
Contributor

I'm confused here, you start off stating that you think length is used to describe offsets... which would indeed be weird (as would size!), but then comment later that you realize it was not being used that way...

It seems to me that some of the documentation is indeed not clear, but it doesn't seem to me that changing length to size makes any difference. You may think they imply some kind of subtle difference, but I don't have that impression. And if you believe the difference between size and length is so clear, perhaps you can describe the difference clearly?

In the absence of a clear and convincing description of why size and length mean different things, I think doc improvements should be focussed on the text which describes the argument's behaviour, not renaming the arguments.

@jalafel
Copy link
Contributor

jalafel commented Nov 25, 2016

@sam-github Continuing from your message from the PR.

About the commit message: That's my bad. I was thinking back to Java, where length usually denotes the data structure (array, list, string) size + 1, counting from 0, thus the index reference. Whereas size would count the items from 1, and denotes the number of items/bits. I agree that there doesn't seem to be a difference in Javascript, and size is not oft used, or rather, used synonymously with length.

I'd be happy to just clarify in the documentation, as opposed to switching out the word choice.

@sam-github
Copy link
Contributor

I didn't know that about java. Yes, I think there is much you can improve in making the text clear, but all the changes in fs don't help, and worse, will make future changes to fs un-backportable to LTS branches. I don't think the length->size change is worth it, but I like your other changes. Sorry, it was probably a lot of work going through fs like that! :-(

@jalafel
Copy link
Contributor

jalafel commented Nov 26, 2016

That is, if I recall correctly from my classes at uni. It may have been specific only to DLL and SLL.

And it's not a problem at all, it wasn't any work :) I will attempt to make the documentation clearer, rather than changing the syntax!

@jorangreef
Copy link
Contributor Author

jorangreef commented Nov 26, 2016 via email

jalafel added a commit to jalafel/node that referenced this issue Nov 27, 2016
This commit clarifies variables in the Filesystem docs.
Prior, the documentation for fs.write() had an ambiguous
remark on the parameters of offset and length.

Therefore, this commit makes explicit that the length parameter
in fs.write() is used to denote the number of bytes, which is
a clearer reference for its usage.

Ref: nodejs#7868
@sam-github
Copy link
Contributor

Length in JS land is almost always an offset into an array or buffer. See array.slice(index, length) for the canonical example..

Its Array.slice(begin, end) in the mozilla docs, not sure which docs you use that describe length being an offset rather than a, well, "length". You'll need to back up your counter-intuitive assertion with some references to be convincing.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/slice

I find no reference to length in the docs other than as a number of sequential elements: https://developer.mozilla.org/en-US/search?q=length

Here's a funny example, https://lodash.com/docs/4.17.2#chunk, you can see the argument is size, and the docs describe size as meaning... "length". Which probably means the arg should have been called length, but since the words are synonyms, shrug.

@sam-github sam-github added doc Issues and PRs related to the documentations. and removed doc Issues and PRs related to the documentations. docs-requested labels Dec 1, 2016
@jalafel
Copy link
Contributor

jalafel commented Jan 15, 2017

@sam-github It's been awhile since this issue has been looked at. The PR has removed the size replacement, and now only extends the definition of length in fs.write().

This is a screenshot from Javascript: The Definitive Guide which uses size to describe length of an array synonymously:

screen shot 2017-01-14 at 7 26 41 pm

I don't know if there's a good argument, unless @jorangreef has something to add. I certainly agree from a language point of view size is a good choice to express a measurement of quantity in bytes. Nonetheless, it might be a large amount of work to backport to the code as well? What do you all think?

italoacasas pushed a commit that referenced this issue Jan 18, 2017
This commit clarifies variables in the Filesystem docs.
Prior, the documentation for fs.write() had an ambiguous
remark on the parameters of offset and length.

Therefore, this commit makes explicit that the length parameter
in fs.write() is used to denote the number of bytes, which is
a clearer reference for its usage.

PR-URL: #9792
Ref: #7868
Reviewed-By: Sam Roberts <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 18, 2017
This commit clarifies variables in the Filesystem docs.
Prior, the documentation for fs.write() had an ambiguous
remark on the parameters of offset and length.

Therefore, this commit makes explicit that the length parameter
in fs.write() is used to denote the number of bytes, which is
a clearer reference for its usage.

PR-URL: nodejs#9792
Ref: nodejs#7868
Reviewed-By: Sam Roberts <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 23, 2017
This commit clarifies variables in the Filesystem docs.
Prior, the documentation for fs.write() had an ambiguous
remark on the parameters of offset and length.

Therefore, this commit makes explicit that the length parameter
in fs.write() is used to denote the number of bytes, which is
a clearer reference for its usage.

PR-URL: nodejs#9792
Ref: nodejs#7868
Reviewed-By: Sam Roberts <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 24, 2017
This commit clarifies variables in the Filesystem docs.
Prior, the documentation for fs.write() had an ambiguous
remark on the parameters of offset and length.

Therefore, this commit makes explicit that the length parameter
in fs.write() is used to denote the number of bytes, which is
a clearer reference for its usage.

PR-URL: nodejs#9792
Ref: nodejs#7868
Reviewed-By: Sam Roberts <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 27, 2017
This commit clarifies variables in the Filesystem docs.
Prior, the documentation for fs.write() had an ambiguous
remark on the parameters of offset and length.

Therefore, this commit makes explicit that the length parameter
in fs.write() is used to denote the number of bytes, which is
a clearer reference for its usage.

PR-URL: nodejs#9792
Ref: nodejs#7868
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 7, 2017
This commit clarifies variables in the Filesystem docs.
Prior, the documentation for fs.write() had an ambiguous
remark on the parameters of offset and length.

Therefore, this commit makes explicit that the length parameter
in fs.write() is used to denote the number of bytes, which is
a clearer reference for its usage.

PR-URL: #9792
Ref: #7868
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 7, 2017
This commit clarifies variables in the Filesystem docs.
Prior, the documentation for fs.write() had an ambiguous
remark on the parameters of offset and length.

Therefore, this commit makes explicit that the length parameter
in fs.write() is used to denote the number of bytes, which is
a clearer reference for its usage.

PR-URL: #9792
Ref: #7868
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
This commit clarifies variables in the Filesystem docs.
Prior, the documentation for fs.write() had an ambiguous
remark on the parameters of offset and length.

Therefore, this commit makes explicit that the length parameter
in fs.write() is used to denote the number of bytes, which is
a clearer reference for its usage.

PR-URL: #9792
Ref: #7868
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
This commit clarifies variables in the Filesystem docs.
Prior, the documentation for fs.write() had an ambiguous
remark on the parameters of offset and length.

Therefore, this commit makes explicit that the length parameter
in fs.write() is used to denote the number of bytes, which is
a clearer reference for its usage.

PR-URL: #9792
Ref: #7868
Reviewed-By: Sam Roberts <[email protected]>
@Trott
Copy link
Member

Trott commented Jul 15, 2017

At least some of this appears to have been addressed. Not sure if there's consensus on the parts that haven't been addressed.

This issue has been inactive for sufficiently long that it seems like perhaps it should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. I'm just tidying up and not acting on a super-strong opinion or anything like that.

@Trott Trott closed this as completed Jul 15, 2017
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.
Projects
None yet
Development

No branches or pull requests

6 participants