doc: add simple example to rename function #18812

punteek · 2018-02-16T09:40:22Z

Added a simple example showing how to rename
a file.

Refs: https://github.com/nodejs/node/issues11135

Checklist

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

Affected core subsystem(s)

doc

tniessen

cc @nodejs/documentation

tniessen · 2018-02-16T11:22:28Z

doc/api/fs.md

+```js
+fs.rename('firstfile.txt', 'secondfile.txt', (err) => {
+	if(err) throw err;
+	console.log("Rename complete!");


Please don't use tabs for indentation, two spaces are appropriate. Also, use single quotes whenever possible ('Rename complete!').

vsemozhetbyt · 2018-02-16T13:14:01Z

doc/api/fs.md

+
+```js
+fs.rename('firstfile.txt', 'secondfile.txt', (err) => {
+	if(err) throw err;


Also if(err) -> if (err) (with a space).

Sorry for the nit. Our doc examples are linted and these tiny things break the linting.

BTW, you can check your doc examples with this command from the repository root:

node tools/node_modules/eslint/bin/eslint.js --ext=.md doc

Trott

Welcome, @punteek, and thanks for the PR! Even small documentation pull requests can generate a lot of comments and requests for changes. Don't get discouraged! I'm using the red-X "Request changes" but the changes I'm requesting are pretty small. (I just think one of them is important enough that I want to make sure it happens before this lands.) Thanks again!

Trott · 2018-02-16T14:54:55Z

doc/api/fs.md

@@ -2529,6 +2529,19 @@ changes:
 Asynchronous rename(2). No arguments other than a possible exception are given
 to the completion callback.

+This function will take the old path name of a file and rename it to the path


In this line and the line below, use pathname (as seen elsewhere in the doc) and not path name.

Trott · 2018-02-16T14:56:40Z

doc/api/fs.md

@@ -2529,6 +2529,19 @@ changes:
 Asynchronous rename(2). No arguments other than a possible exception are given
 to the completion callback.

+This function will take the old path name of a file and rename it to the path
+name provided in the second parameter.


Rather than saying "old path name" and "second parameter", please use the argument names in backticks: `oldPath` and `newPath`.

Trott · 2018-02-16T14:58:08Z

doc/api/fs.md

+This function will take the old path name of a file and rename it to the path
+name provided in the second parameter.
+
+Here is an example below (assuming the file "firstfile.txt" already exists):


No need to say "Here is an example below..." Just provide the example. If you want it to be clear what the example does, add a code comment to the example along the lines of:

// Renames firstfile.txt to secondfile.txt

timotew

Thanks for the PR! 👍

timotew · 2018-02-16T15:38:47Z

doc/api/fs.md

+});
+```
+
+


Welcome @punteek.
Nit: multiple lines are actually going to be shown as a single line in the output.

timotew · 2018-02-16T15:47:26Z

doc/api/fs.md

+This function will take the old path name of a file and rename it to the path
+name provided in the second parameter.
+
+Here is an example below (assuming the file "firstfile.txt" already exists):


Like @Trott has stated it's better to use backticks(`) for arguments/variable names in doc. "firstfile.txt" -> `firstfile.txt`

punteek · 2018-02-16T23:03:07Z

Thanks everyone! I've made the suggested updates, please let me know if anything else is needed.

vsemozhetbyt · 2018-02-16T23:07:03Z

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

(Sorry, closed by mistake)

TimothyGu

Thanks for doing this. A few minor comments before it gets landed.

TimothyGu · 2018-02-16T23:40:40Z

doc/api/fs.md

@@ -2529,6 +2529,18 @@ changes:
 Asynchronous rename(2). No arguments other than a possible exception are given
 to the completion callback.

+This function will take the `oldPath` of a file and rename it to the
+pathname provided as `newPath`.


How about making something to this effect the first sentence of the documentation, rather than making the user have to read to the second paragraph? Something like the following would sound good to me, and be consistent with the rest of the file (c.f. chmod()).

Take the `oldPath` of a file and rename it to the pathname provided as `newPath` asynchronously. No arguments other than a possible exception are given to the completion callback. See also: rename(2).

Also, the documentation could be more helpful if it specifies what happens if newPath already exists as a file, or directory. Is the callback function called with an exception, or is the original file overwritten?

Further edit to @TimothyGu's suggestion to make the first sentence more concise:

Asynchronously rename file at `oldPath` to the pathname provided as `newPath`. No arguments other than a possible exception are given to the completion callback. See also: rename(2).

Trott · 2018-02-17T06:04:58Z

doc/api/fs.md

+This function will take the `oldPath` of a file and rename it to the
+pathname provided as `newPath`.
+
+Example renames 'firstfile.txt' to 'secondfile.txt':


Again, telling people that an example follows is unnecessary. Just provide the example. Remove this line.

BridgeAR · 2018-02-21T17:00:31Z

Ping @punteek

punteek · 2018-02-22T00:44:57Z

Sorry for the delay; I have again made the requested updates.

mmarchini · 2018-02-22T14:46:37Z

New CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/191/

stale review

devsnek · 2018-02-22T22:18:31Z

firstfile and secondfile are confusing names, perhaps currentName and newName?

Trott · 2018-02-22T23:41:19Z

firstfile and secondfile are confusing names, perhaps currentName and newName?

Or old/oldfile and new/newfile?

BridgeAR · 2018-03-02T01:18:48Z

Or even more verbose: oldFileName and newFileName.

BridgeAR · 2018-03-02T01:19:04Z

@punteek would you be so kind and have another look? :-)

Added a simple example showing how to rename a file. Refs: https://github.com/nodejs/node/issues11135

punteek · 2018-03-05T00:40:03Z

I've updated the names of the files to "oldFile.txt" and "newFile.txt".

mmarchini · 2018-03-05T14:10:32Z

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

mmarchini · 2018-03-05T19:53:10Z

Landed in 1d2ab79 😄

Added a simple example showing how to rename a file. Refs: https://github.com/nodejs/node/issues11135 PR-URL: #18812 Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

punteek · 2018-03-06T18:45:01Z

Thank you all for your suggestions and help on this issue. This is my first open source contribution and I had a great experience!

BridgeAR · 2018-03-06T19:06:04Z

@punteek I am glad to hear that 😃. Do you have any feedback about what you likes most / what should maybe be handled better?

Added a simple example showing how to rename a file. Refs: https://github.com/nodejs/node/issues11135 PR-URL: #18812 Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

Added a simple example showing how to rename a file. Refs: https://github.com/nodejs/node/issues11135 PR-URL: nodejs#18812 Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

Added a simple example showing how to rename a file. Refs: https://github.com/nodejs/node/issues11135 Backport-PR-URL: #22380 PR-URL: #18812 Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

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

tniessen reviewed Feb 16, 2018

View reviewed changes

vsemozhetbyt reviewed Feb 16, 2018

View reviewed changes

Trott previously requested changes Feb 16, 2018

View reviewed changes

timotew reviewed Feb 16, 2018

View reviewed changes

punteek force-pushed the my-branch branch 2 times, most recently from 54a458d to ee3a6b6 Compare February 16, 2018 22:59

vsemozhetbyt closed this Feb 16, 2018

vsemozhetbyt reopened this Feb 16, 2018

TimothyGu reviewed Feb 16, 2018

View reviewed changes

Trott reviewed Feb 17, 2018

View reviewed changes

punteek force-pushed the my-branch branch from ee3a6b6 to 4988414 Compare February 22, 2018 00:43

punteek force-pushed the my-branch branch from 4988414 to b9eb745 Compare February 22, 2018 07:13

mmarchini approved these changes Feb 22, 2018

View reviewed changes

TimothyGu approved these changes Feb 22, 2018

View reviewed changes

doc: add simple example to rename function

6e934b7

Added a simple example showing how to rename a file. Refs: https://github.com/nodejs/node/issues11135

punteek force-pushed the my-branch branch from b9eb745 to 6e934b7 Compare March 5, 2018 00:38

tniessen approved these changes Mar 5, 2018

View reviewed changes

gibfahn approved these changes Mar 5, 2018

View reviewed changes

jasnell approved these changes Mar 5, 2018

View reviewed changes

mmarchini closed this Mar 5, 2018

MylesBorins mentioned this pull request Mar 6, 2018

v9.8.0 proposal #19181

Merged

MylesBorins mentioned this pull request Sep 6, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: add simple example to rename function #18812

doc: add simple example to rename function #18812

punteek commented Feb 16, 2018

tniessen left a comment

tniessen Feb 16, 2018

vsemozhetbyt Feb 16, 2018 •

edited

Loading

Trott left a comment

Trott Feb 16, 2018

Trott Feb 16, 2018

Trott Feb 16, 2018

timotew left a comment

timotew Feb 16, 2018

timotew Feb 16, 2018

punteek commented Feb 16, 2018

vsemozhetbyt commented Feb 16, 2018 •

edited

Loading

TimothyGu left a comment

TimothyGu Feb 16, 2018 •

edited

Loading

Trott Feb 17, 2018

Trott Feb 17, 2018

BridgeAR commented Feb 21, 2018

punteek commented Feb 22, 2018

mmarchini commented Feb 22, 2018

devsnek commented Feb 22, 2018

Trott commented Feb 22, 2018

BridgeAR commented Mar 2, 2018

BridgeAR commented Mar 2, 2018

punteek commented Mar 5, 2018

mmarchini commented Mar 5, 2018

mmarchini commented Mar 5, 2018

punteek commented Mar 6, 2018

BridgeAR commented Mar 6, 2018

		});
		```

doc: add simple example to rename function #18812

doc: add simple example to rename function #18812

Conversation

punteek commented Feb 16, 2018

Checklist

Affected core subsystem(s)

tniessen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

punteek commented Feb 16, 2018

vsemozhetbyt commented Feb 16, 2018 • edited Loading

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Feb 21, 2018

punteek commented Feb 22, 2018

mmarchini commented Feb 22, 2018

devsnek commented Feb 22, 2018

Trott commented Feb 22, 2018

BridgeAR commented Mar 2, 2018

BridgeAR commented Mar 2, 2018

punteek commented Mar 5, 2018

mmarchini commented Mar 5, 2018

mmarchini commented Mar 5, 2018

punteek commented Mar 6, 2018

BridgeAR commented Mar 6, 2018

vsemozhetbyt Feb 16, 2018 •

edited

Loading

vsemozhetbyt commented Feb 16, 2018 •

edited

Loading

TimothyGu Feb 16, 2018 •

edited

Loading