Skip to content

doc: update behaviour of fs.writeFile#25080

Closed
thefourtheye wants to merge 1 commit into
nodejs:masterfrom
thefourtheye:update-fs-writefile-doc
Closed

doc: update behaviour of fs.writeFile#25080
thefourtheye wants to merge 1 commit into
nodejs:masterfrom
thefourtheye:update-fs-writefile-doc

Conversation

@thefourtheye
Copy link
Copy Markdown
Contributor

@thefourtheye thefourtheye commented Dec 17, 2018

As per the decision in #23433,
the fs.writeFile will always write from the current position if it
is used with file descriptors. This patch updates it.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

cc @nodejs/fs @nodejs/tsc

@thefourtheye thefourtheye added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version. labels Dec 17, 2018
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Dec 17, 2018

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

@thefourtheye thefourtheye mentioned this pull request Dec 17, 2018
Closed
mcollina
mcollina approved these changes Dec 17, 2018
View reviewed changes
Copy link
Copy Markdown
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

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 17, 2018
@vsemozhetbyt
Copy link
Copy Markdown
Contributor

vsemozhetbyt commented Dec 17, 2018

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

rvagg
rvagg approved these changes Dec 18, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

pls make sure there's a Ref: https://github.com/nodejs/node/pull/23709 in the metadata too.

@sam-github
Copy link
Copy Markdown
Contributor

sam-github commented Dec 18, 2018

I don't think it should be fast-tracked. Its a critical, much debated, long discussed change, and documentation text should at least have the opportunity for people to comment on it.

sam-github
sam-github reviewed Dec 18, 2018
View reviewed changes
Comment thread doc/api/fs.md Outdated
Copy link
Copy Markdown
Contributor

@sam-github sam-github Dec 18, 2018

Choose a reason for hiding this comment

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

I would suggest a string of ', World'. Or perhaps, the first string could be 'Dogs\n', and the second 'Cats\n'? The docs as written generate a misspelled phrase, which to me suggests its demonstrating a side-effect of some kind of misuse of the API, rather than a working-as-designed-and-useful capability.

Copy link
Copy Markdown
Contributor Author

@thefourtheye thefourtheye Dec 26, 2018

Choose a reason for hiding this comment

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

I included your suggestion, the , World. I didn't change it to Dogs and Cats because, readFile also uses Hello World as the example. Perhaps we can change them together in a separate PR. What do you think?

trivikr
trivikr approved these changes Dec 23, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@trivikr trivikr left a comment

Choose a reason for hiding this comment

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

Related code change #23709

@thefourtheye thefourtheye force-pushed the update-fs-writefile-doc branch 2 times, most recently from 3177ae2 to bb31222 Compare December 26, 2018 07:38
@thefourtheye
doc: update behaviour of fs.writeFile
55822cf 
As per the decision in nodejs#23433,
the `fs.writeFile` will always write from the current position if it
is used with file descriptors. This patch updates it.

Ref: nodejs#23709
@thefourtheye thefourtheye force-pushed the update-fs-writefile-doc branch from bb31222 to 55822cf Compare December 26, 2018 07:40
@addaleax
Copy link
Copy Markdown
Member

addaleax commented Jan 7, 2019

Landed in 309e772

@addaleax addaleax closed this Jan 7, 2019
addaleax pushed a commit that referenced this pull request Jan 7, 2019
@thefourtheye @addaleax
doc: update behaviour of fs.writeFile
309e772 
As per the decision in #23433,
the `fs.writeFile` will always write from the current position if it
is used with file descriptors. This patch updates it.

Ref: #23709

PR-URL: #25080
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@thefourtheye thefourtheye deleted the update-fs-writefile-doc branch January 8, 2019 03:52
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@thefourtheye @refack
doc: update behaviour of fs.writeFile
ab5806d 
As per the decision in nodejs#23433,
the `fs.writeFile` will always write from the current position if it
is used with file descriptors. This patch updates it.

Ref: nodejs#23709

PR-URL: nodejs#25080
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Apr 18, 2019
Merged
BethGriggs added a commit that referenced this pull request Apr 22, 2019
@BethGriggs