doc: clarify fd behaviour with {read,write}File by thefourtheye · Pull Request #23706 · nodejs/node

thefourtheye · 2018-10-17T07:30:09Z

Checklist

documentation is changed or added
commit message follows commit guidelines

cc @nodejs/fs @nodejs/documentation @Trott

nodejs-github-bot · 2018-10-17T07:30:11Z

@thefourtheye build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1271/pipeline

vsemozhetbyt

With nit suggestions.

vsemozhetbyt · 2018-10-17T14:41:13Z

doc/api/fs.md

 automatically.
+3. The reading will begin at the current position. If the file size is
+10 bytes and if six bytes are already read with this file descriptor, then
+`readFile` will return only the rest of the four bytes.


Nit:

Suggested change

`readFile` will return only the rest of the four bytes.

`readFile()` will return only the rest of the four bytes.

vsemozhetbyt · 2018-10-17T14:45:51Z

doc/api/fs.md

+automatically.
+3. The writing will begin at the beginning of the file. If the file size
+is 10 bytes and if six bytes are written with this file descriptor, then
+`writeFile` will return six bytes newly written and four bytes from the file.


This seems a bit confusing as writeFileSync() returns undefined and writeFile() transfers only error to the calback. Maybe something like this this?

Suggested change

`writeFile` will return six bytes newly written and four bytes from the file.

the file contents would be six bytes newly written and four bytes from the file.

vsemozhetbyt · 2018-10-17T14:54:26Z

Should we also update filehandle.readFile(), filehandle.writeFile(), fsPromises.readFile(), and fsPromises.writeFile() docs? Or this should be done after the #23709 ?

thefourtheye · 2018-10-17T15:19:12Z

@vsemozhetbyt If #23709 lands, behavior of writeFile has to be updated here.

thefourtheye · 2018-10-19T23:02:42Z

Now that #23709 is gearing up to land, I want this PR also to be landed along with that. So, I have made changes to the writeFile* documentation according to that. @nodejs/fs and the reviewers PTAL again.

vsemozhetbyt · 2018-10-19T23:38:13Z

doc/api/fs.md

+3. The writing will begin at the current position. Basically, if there are
+multiple write calls to a file descriptor and then a `writeFile()` call is
+made with the same file descriptor, the data would have been appended.
+For example, if we did something like this


Maybe this line can be omitted as self-evident.

vsemozhetbyt · 2018-10-19T23:49:27Z

I am in doubt for these fragments of write counterparts:

the data would have been appended.

the data will be written from the current position till the end of the file.

This may be true only if the current position is the end of the file or the newly written data is less then the old rewritten data.

Say, we have HelloWorld and the current position is after Hello. If we write 1, would not the content be Hello1orld? If so, neither of notes is true,

thefourtheye · 2018-10-20T11:51:51Z

@vsemozhetbyt Once #23709 lands, the behaviour will become like this

➜  node git:(fix-fs-writeFile-with-fd) echo "Hello World" > /tmp/test
➜  node git:(fix-fs-writeFile-with-fd) cat ~/Desktop/Scratch/Test.js

'use strict';
const fs = require('fs');
const fileName = '/tmp/test';
const fd = fs.openSync(fileName, 'w');

fs.writeFile(fd, 'Hello', function(err) {
  if (err) {
    throw err;
  }
  fs.writeFile(fd, 'World', function(err) {
    if (err) {
      throw err;
    }
    console.log(fs.readFileSync(fileName).toString());
  });
});

➜  node git:(fix-fs-writeFile-with-fd) ./node ~/Desktop/Scratch/Test.js
HelloWorld
➜  node git:(fix-fs-writeFile-with-fd)

Even if the file has contents, the first write will always truncate the file. So, it will always seem like appending only.

vsemozhetbyt · 2018-10-20T12:06:02Z

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1316/

mcollina

LGTM

ChALkeR · 2018-10-24T12:13:45Z

doc/api/fs.md

+1. Any specified file descriptor has to support reading.
+2. If a file descriptor is specified as the `path`, it will not be closed
+automatically.
+3. The reading will begin at the current position. If the file size is


Clarify that the If ... is an example?
Something like E.g., if ... or For example, if ....

ChALkeR · 2018-10-24T12:15:34Z

doc/api/fs.md


 The `FileHandle` has to support reading.

+**Note:** If one or more `filehandle.read()` calls are made on a file handle


We deliberately don't use **Note:** anymore afaik?

See #18592.

It's not official, but there has definitely been an effort to use it much less. In this case, I think It can be dropped and you can just say what you have to say:

If one or more `filehandle.read()` calls are made on a file handle

Makes sense. I removed the Note now.

ChALkeR · 2018-10-24T12:18:02Z

doc/api/fs.md

 It is unsafe to use `filehandle.writeFile()` multiple times on the same file
 without waiting for the `Promise` to be resolved (or rejected).

+**Note:** If one or more `filehandle.write()` calls are made on a file handle


ChALkeR

LGTM with nits.

mhdawson

LGTM once comments are addressed.

This reverts commit b5a440c.

thefourtheye · 2018-10-24T13:21:42Z

I reverted the changes necessary for #23709 and addressed the review comments. Reviewers, PTAL again.

thefourtheye · 2018-10-25T15:03:30Z

I'll land this by 31 October 2018, 07:30:00 (UTC time), if there are no objections or suggestions.

Trott · 2018-10-25T17:37:02Z

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1368/

thefourtheye · 2018-10-31T14:02:40Z

Landed in 8bd6df8.

PR-URL: #23706 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

doc: clarify fd behaviour with {read,write}File

351d9df

nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Oct 17, 2018

thefourtheye mentioned this pull request Oct 17, 2018

fs: behaviour of readFile and writeFile with file descriptors #23433

Closed

addaleax approved these changes Oct 17, 2018

View reviewed changes

lpinca approved these changes Oct 17, 2018

View reviewed changes

vsemozhetbyt approved these changes Oct 17, 2018

View reviewed changes

address @vsemozhetbyt's comments

5370c14

vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 19, 2018

trivikr approved these changes Oct 19, 2018

View reviewed changes

thefourtheye added 3 commits October 20, 2018 04:21

changes because of nodejs#23709

b5a440c

group the relevant parts

d44bf9f

update fs promisified functions' behaviour

121e1dd

vsemozhetbyt reviewed Oct 19, 2018

View reviewed changes

mcollina approved these changes Oct 22, 2018

View reviewed changes

ChALkeR reviewed Oct 24, 2018

View reviewed changes

ChALkeR approved these changes Oct 24, 2018

View reviewed changes

mhdawson approved these changes Oct 24, 2018

View reviewed changes

thefourtheye added 2 commits October 24, 2018 18:36

Revert "changes because of nodejs#23709"

9975e9a

This reverts commit b5a440c.

address review comments

ef290a7

thefourtheye closed this Oct 31, 2018

thefourtheye deleted the update-rw-file-docs-with-fd branch October 31, 2018 14:03

targos mentioned this pull request Nov 1, 2018

Release proposal: v11.1.0 #23922

Merged

MylesBorins added dont-land-on-v8.x labels Nov 27, 2018

codebytere mentioned this pull request Nov 27, 2018

v10.13.1 proposal #24675

Closed

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged

	`readFile` will return only the rest of the four bytes.
	`readFile()` will return only the rest of the four bytes.

	`writeFile` will return six bytes newly written and four bytes from the file.
	the file contents would be six bytes newly written and four bytes from the file.


		The `FileHandle` has to support reading.

		Note: If one or more `filehandle.read()` calls are made on a file handle

Uh oh!

Conversation

thefourtheye commented Oct 17, 2018

Checklist

Uh oh!

nodejs-github-bot commented Oct 17, 2018

Uh oh!

vsemozhetbyt left a comment

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!

vsemozhetbyt commented Oct 17, 2018

Uh oh!

thefourtheye commented Oct 17, 2018

Uh oh!

thefourtheye commented Oct 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thefourtheye commented Oct 20, 2018

Uh oh!

vsemozhetbyt commented Oct 20, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ChALkeR Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChALkeR Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ChALkeR left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Oct 24, 2018

Uh oh!

thefourtheye commented Oct 25, 2018

Uh oh!

Trott commented Oct 25, 2018

Uh oh!

thefourtheye commented Oct 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

vsemozhetbyt commented Oct 19, 2018 •

edited

Loading

ChALkeR Oct 24, 2018 •

edited

Loading

ChALkeR Oct 24, 2018 •

edited

Loading