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

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

Closed

Conversation

thefourtheye
Copy link
Contributor

Checklist

Ref: #23433 (comment)

cc @nodejs/fs @nodejs/documentation @Trott

@nodejs-github-bot 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
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

With nit suggestions.

doc/api/fs.md Outdated
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

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

doc/api/fs.md Outdated
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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

@thefourtheye
Copy link
Contributor Author

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

@vsemozhetbyt 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
@thefourtheye
Copy link
Contributor Author

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.

doc/api/fs.md Outdated
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this line can be omitted as self-evident.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Oct 19, 2018

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
Copy link
Contributor Author

@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
Copy link
Contributor

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

doc/api/fs.md Outdated
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
Copy link
Member

@ChALkeR ChALkeR Oct 24, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack.

doc/api/fs.md Outdated
@@ -3780,6 +3801,11 @@ returned.

The `FileHandle` has to support reading.

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

@ChALkeR ChALkeR Oct 24, 2018

Choose a reason for hiding this comment

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

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

See #18592.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I removed the Note now.

doc/api/fs.md Outdated
@@ -3942,6 +3968,11 @@ The `FileHandle` has to support writing.
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
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

LGTM with nits.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM once comments are addressed.

@thefourtheye
Copy link
Contributor Author

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

@thefourtheye
Copy link
Contributor Author

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

@Trott
Copy link
Member

Trott commented Oct 25, 2018

@thefourtheye
Copy link
Contributor Author

Landed in 8bd6df8.

@thefourtheye thefourtheye deleted the update-rw-file-docs-with-fd branch October 31, 2018 14:03
thefourtheye added a commit that referenced this pull request Oct 31, 2018
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>
targos pushed a commit that referenced this pull request Nov 1, 2018
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>
MylesBorins pushed a commit that referenced this pull request Nov 27, 2018
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>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
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>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.