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.

Sign up for GitHub

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

Clarify "fs.readFile", "fs.appendFile", "fs.writeFile" doc… #7561

Closed
wants to merge 5 commits into from
Closed

Clarify "fs.readFile", "fs.appendFile", "fs.writeFile" doc… #7561

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e4a8d56
refs #7560 Clarify "fs.readFile", "fs.appendFile", "fs.writeFile" doc…
kibertoad Jul 6, 2016
a851058
refs #7560 Use backticks instead of single quotes.
kibertoad Jul 6, 2016
40a5750
refs #7560 Merge two notes, wrap at 80 symbols.
kibertoad Jul 6, 2016
7e617e0
refs #7560 Add missing "the".
kibertoad Jul 9, 2016
b5baaa2
refs #7560 Removed excessive detail about internal implementation.
kibertoad Jul 17, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions doc/api/fs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,8 @@ fs.appendFile('message.txt', 'data to append', 'utf8', callback);

Any specified file descriptor has to have been opened for appending.

_Note: Specified file descriptors will not be closed automatically._
_Note: If a file descriptor is specified as the `file`, it will not be closed
automatically._
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove automatically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was in the original text. It makes perfect sense. It's just one word and doesn't overload text or can be a source of confusion. Why does it have to be removed?..

Copy link
Member

Choose a reason for hiding this comment

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

I’m +1 on keeping it, too. The file descriptor obviously should be closed at some point, that just doesn’t happen by calling {append,read,write}File.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does that mean that this PR is finally in a proper shape :)?

Copy link
Member

Choose a reason for hiding this comment

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

That’s what my “LGTM” (= looks good to me) basically says, yes. :) It’s still part of the process of merging PRs that collaborators have the chance to state their opinion on changes, like @thefourtheye did, and discuss them if necessary.

Also, you could do us a favour by squashing the commits here together and formatting the commit message as requested by the CONTRIBUTING.md (that’s also something that can be done by the person landing this PR).

Copy link
Member

Choose a reason for hiding this comment

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

ping @thefourtheye?


## fs.appendFileSync(file, data[, options])
<!-- YAML
Expand Down Expand Up @@ -1130,7 +1131,8 @@ fs.readFile('/etc/passwd', 'utf8', callback);

Any specified file descriptor has to support reading.

_Note: Specified file descriptors will not be closed automatically._
_Note: If a file descriptor is specified as the `file`, it will not be closed
automatically._

## fs.readFileSync(file[, options])
<!-- YAML
Expand Down Expand Up @@ -1662,7 +1664,8 @@ Note that it is unsafe to use `fs.writeFile` multiple times on the same file
without waiting for the callback. For this scenario,
`fs.createWriteStream` is strongly recommended.

_Note: Specified file descriptors will not be closed automatically._
_Note: If a file descriptor is specified as the `file`, it will not be closed
automatically._

## fs.writeFileSync(file, data[, options])
<!-- YAML
Expand Down