clarify the position argument for fs.read #14612
Conversation
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
|
@dcharbonnier thanks for following up 👍 |
|
@vsemozhetbyt, @refack :-) |
|
let me change it @Trott |
|
@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. |
| @@ -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. | |||
| 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. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
@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.
| @@ -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. | |||
There was a problem hiding this comment.
I suggest "is an argument", its not always an integer, null is not an integer
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Looks like that PR landed in libuv 1.13.0, so it has at least made its way to Node.
There was a problem hiding this comment.
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. |
|
@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. |
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), orvcbuild test(Windows) passesAffected core subsystem(s)