clarify the position argument for fs.read by dcharbonnier · Pull Request #14612 · nodejs/node · GitHub

What happen to the file position after a read using a position null or integer was not clear and you can assume that the cursor of the file descriptor is updated even if position is an integer.
close #8397

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

What happen to the file position after a read using a position null or integer was not clear and you can assume that the cursor of the file descriptor is updated even if position is an integer. close #8397

@dcharbonnier thanks for following up 👍

@vsemozhetbyt, @refack :-)

LGTM. I think subsequent read would be better as subsequent reads but either way is 👍 with me.

let me change it @Trott

Thank you!

@dcharbonnier Just in case you did not know yet, you can just change the file and push to the same branch again to update a PR, you don't need to open a new one :)

yes @tniessen , I was editing directly on github what I never do and I didn't get that it was creating a branch on a fork, nothing magic

@dcharbonnier just in case you haven't yet, you might find it informative to take a look the the CONTRIBUTING guide.
PRs are kept open for at least 48 hours so that everyone has a chance to review. And as I mentioned before documentation changes tend to get more attention because they can't be machine tested, so human eyes are the only way to verify them. But from the list of approvers it seems like this one should go smoothly 😉

There is only one current file position, its used for read and for write, but this makes it sound like its used only for reads. I would just say the current file position is updated.

It's pretty obvious that there is only one file position, this "for subsequent reads" was to ring a bell.
You can put "operation" instead of "read" but in this case it will ring no bell.
What need to be clarified is that what happen to the position in the descriptor is different if the value is null, the documentation was saying nothing about this and so I (and I'm not the only one, see the issue) was assuming that the behavior was the same in both cases.
The current documentation is not saying anything about the fact that there is only one file position, I don't get why we should start mentioning it. How many times do you do a read/write ? Don't you thing that when people do this they already have a pretty god idea of the fact that there is one cursor in the file descriptor ? And that they would find strange that there would be one for read and one for write ?

@sam-github's concern here can be addressed with a simple edit here...

If `position` is `null`, data will be read from the current file position,
and the file position will be updated.

I suggest "is an argument", its not always an integer, null is not an integer

#14621

maybe it feels obvious, but for null you say where it reads from (current position) and what happens to current position (its updated). For the integer case this says file position is unchange, but it doesn't say where read is from, which is from the integer as a positive offset from beginning of file (-4 won't read from 4 bytes before end of file, for example).

also, the file position will be changed on Windows, though there is a UV PR to change windows to be like unix: libuv/libuv#1357, not sure if that has effected node yet, though.

Looks like that PR landed in libuv 1.13.0, so it has at least made its way to Node.

yeah and let's say what happen if you have a position that goes over the size of the file and specify that it's bytes, also say that the position in the end is the quantity that has been read + the previous position but let's wait that you do it on an other PR because this has noting to do with this one

@sam-github good comments, and obviously if @dcharbonnier follows up that would be great, but could you clarify your general outlook on this.
Could this land as is and the comments addressed in the future?

LGTM with Sam's comments addressed.

@dcharbonnier IMHO discussion is a good part of the process. Also "addressing" comments can mean simply explain why a certain decision was made, just like you did.
Usually when a reviewer see something that is wrong they will use the "Request Changes" button, or explicitly write "-1".

clarify the position argument for fs.read

04d1ca0

What happen to the file position after a read using a position null or integer was not clear and you can assume that the cursor of the file descriptor is updated even if position is an integer. close #8397

nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Aug 3, 2017

refack self-assigned this Aug 3, 2017

addaleax approved these changes Aug 3, 2017

View reviewed changes

Trott approved these changes Aug 3, 2017

View reviewed changes

refack approved these changes Aug 3, 2017

View reviewed changes

vsemozhetbyt approved these changes Aug 3, 2017

View reviewed changes

tniessen approved these changes Aug 3, 2017

View reviewed changes

evanlucas approved these changes Aug 3, 2017

View reviewed changes

fix typo

ad9134f

kfarnung approved these changes Aug 3, 2017

View reviewed changes

lpinca approved these changes Aug 3, 2017

View reviewed changes

sam-github reviewed Aug 3, 2017

View reviewed changes

cjihrig approved these changes Aug 4, 2017

View reviewed changes

dcharbonnier closed this Aug 4, 2017

Trott mentioned this pull request Aug 4, 2017

doc: clarify fs.read() #14631

Closed

3 tasks

refack removed their assignment Oct 20, 2018

               `position` is an integer specifying where to begin reading from in the file.
-              If `position` is `null`, data will be read from the current file position.
+              If `position` is `null`, data will be read from the current file position,
+              and the file position will be updated for subsequent reads.

-              If `position` is `null`, data will be read from the current file position.
+              If `position` is `null`, data will be read from the current file position,
+              and the file position will be updated for subsequent reads.
+              If `position` is an integer, the file position will remain unchanged.

		@@ -1640,7 +1640,9 @@ Read data from the file specified by `fd`.
		`length` is an integer specifying the number of bytes to read.

		`position` is an integer specifying where to begin reading from in the file.

clarify the position argument for fs.read by dcharbonnier · Pull Request #14612 · nodejs/node · GitHub

Uh oh!

Uh oh!

clarify the position argument for fs.read - #14612

clarify the position argument for fs.read#14612dcharbonnier wants to merge 2 commits intonodejs:masternodejs/node:masterfrom dcharbonnier:patch-4Copy head branch name to clipboard

Conversation

dcharbonnier commented Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

refack commented Aug 3, 2017

Uh oh!

dcharbonnier commented Aug 3, 2017

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

dcharbonnier commented Aug 3, 2017

Uh oh!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen commented Aug 3, 2017

Uh oh!

dcharbonnier commented Aug 3, 2017

Uh oh!

refack commented Aug 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 3, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

clarify the position argument for fs.read#14612
dcharbonnier wants to merge 2 commits into
nodejs:masterfrom
dcharbonnier:patch-4

dcharbonnier commented Aug 3, 2017 •
edited
Loading