doc: clarify fs.link and fs.linkSync arguments by kemitchell · Pull Request #9145 · nodejs/node

kemitchell · 2016-10-18T03:19:40Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

fs and doc

(Edited this answer 2016-10-19T20:46+0000 to show commits patch both fs and fs doc.)

Description of change

Clarifies documentation by replacing the fs.link and fs.linkSync argument names srcpath and dstpath with more descriptive existingpath and newpath, reflecting how POSIX describes link().

A few double-takes on my part inspired this small PR. As you're creating a new hard link, the "destination" seems like the new path, until it's actually done, when the "destination" of the new link seems like the prior-existing path.

The Linux manpage currently linked from doc names these arguments oldpath and newpath. A FreeBSD manpage I found online says name1 and name2. POSIX' listing calls them path1 and path2. Since there's no hope of matching the manpage on every system, I went with the most descriptive names I could come up with, alluding to the POSIX explanatory text.

Trott · 2016-10-18T03:50:15Z

Nit: Do we want to rename them in the code as well? The functions are pretty short so it shouldn't be too difficult/risky.

mscdex · 2016-10-18T04:09:33Z

The individual parameter names below the function signature need to be updated as well.

Slightly offtopic nit: we should use camel casing (existingPath vs existingpath), but the rest of the document doesn't seem to use it... :(

kemitchell · 2016-10-19T18:40:55Z

use camel case
amend commit message to match
update argument descriptions below signatures

kemitchell · 2016-10-19T18:41:23Z

I've also "Allow[ed] edits from maintainers." here on GitHub.

Trott · 2016-10-19T19:04:42Z

Anybody have an opinion on this?:

Do we want to rename them in the code as well? The functions are pretty short so it shouldn't be too difficult/risky.

I think we mostly have the parameter names listed in the docs matching the parameters in the code...

gibfahn · 2016-10-19T19:36:47Z

I like these new names, it does seem more clear. If we change the docs I guess it would be inconsistent not to change the code to match.

I can't see how changing the names would affect any code using these functions, so it should be straightforward to change. Is there any possibility of parameter name change causing breakage?

thefourtheye · 2016-10-19T19:56:07Z

doc/api/fs.md

What is the change in this line?

kemitchell · 2016-10-19T19:59:32Z

My editor probably added a new line to the end of the file that wasn't there before. I can remove to clean up the patch.

Clarifies documentation by replacing the argument names `srcpath` and `dstpath` with more descriptive `existingPath` and `newPath`, reflecting how POSIX describes `link()`.

kemitchell · 2016-10-19T20:40:28Z

remove newline added to end of file

Updates the argument names `srcpath` and `dstpath` to match the more descriptive `existingPath` and `newPath` in the documentation.

kemitchell · 2016-10-19T20:45:13Z

@Trott I pushed an additional commit changing argument names in lib/fs.js. Not sure whether the team would prefer these commits as PR'd, these commits reordered, or a single squashed fs: ... commit. Please feel free to amend as appropriate.

lpinca

LGTM

Trott · 2016-10-21T04:10:04Z

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

gibfahn

LGTM

Clarifies documentation by replacing the argument names `srcpath` and `dstpath` with more descriptive `existingPath` and `newPath`, reflecting how POSIX describes `link()`. PR-URL: #9145 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Updates the argument names `srcpath` and `dstpath` to match the more descriptive `existingPath` and `newPath` in the documentation. PR-URL: #9145 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

jasnell · 2016-10-21T15:02:59Z

landed in 6f7cdf7 and e8fadd8

Clarifies documentation by replacing the argument names `srcpath` and `dstpath` with more descriptive `existingPath` and `newPath`, reflecting how POSIX describes `link()`. PR-URL: #9145 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Updates the argument names `srcpath` and `dstpath` to match the more descriptive `existingPath` and `newPath` in the documentation. PR-URL: #9145 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

MylesBorins · 2016-11-15T18:50:18Z

should this be backported? feel free to update the labels

Clarifies documentation by replacing the argument names `srcpath` and `dstpath` with more descriptive `existingPath` and `newPath`, reflecting how POSIX describes `link()`. PR-URL: #9145 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Updates the argument names `srcpath` and `dstpath` to match the more descriptive `existingPath` and `newPath` in the documentation. PR-URL: #9145 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Clarifies documentation by replacing the argument names `srcpath` and `dstpath` with more descriptive `existingPath` and `newPath`, reflecting how POSIX describes `link()`. PR-URL: #9145 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Updates the argument names `srcpath` and `dstpath` to match the more descriptive `existingPath` and `newPath` in the documentation. PR-URL: #9145 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

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 18, 2016

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

kemitchell force-pushed the clarify-fs-link-argument-docs branch 2 times, most recently from ab688c7 to 4bbf3ca Compare October 19, 2016 18:40

thefourtheye reviewed Oct 19, 2016

View reviewed changes

doc/api/fs.md Outdated

Copy link

Contributor

thefourtheye Oct 19, 2016

Choose a reason for hiding this comment

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

What is the change in this line?

doc: clarify fs.link and fs.linkSync arguments

5618ae2

Clarifies documentation by replacing the argument names `srcpath` and `dstpath` with more descriptive `existingPath` and `newPath`, reflecting how POSIX describes `link()`.

kemitchell force-pushed the clarify-fs-link-argument-docs branch from 4bbf3ca to 5618ae2 Compare October 19, 2016 20:38

fs: clarify fs.link and fs.linkSync arguments

ecacc61

Updates the argument names `srcpath` and `dstpath` to match the more descriptive `existingPath` and `newPath` in the documentation.

thefourtheye approved these changes Oct 20, 2016

View reviewed changes

jasnell approved these changes Oct 20, 2016

View reviewed changes

lpinca approved these changes Oct 20, 2016

View reviewed changes

gibfahn approved these changes Oct 21, 2016

View reviewed changes

jasnell self-assigned this Oct 21, 2016

jasnell closed this Oct 21, 2016

kemitchell deleted the clarify-fs-link-argument-docs branch October 21, 2016 15:53

MylesBorins added dont-land-on-v4.x labels Nov 15, 2016

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Dec 21, 2016

MylesBorins mentioned this pull request Dec 21, 2016

V6.9.3 proposal #10394

Merged

Uh oh!

Conversation

kemitchell commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Trott commented Oct 18, 2016

Uh oh!

mscdex commented Oct 18, 2016

Uh oh!

kemitchell commented Oct 19, 2016

Uh oh!

kemitchell commented Oct 19, 2016

Uh oh!

Trott commented Oct 19, 2016

Uh oh!

gibfahn commented Oct 19, 2016

Uh oh!

thefourtheye Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

kemitchell commented Oct 19, 2016 via email

Uh oh!

kemitchell commented Oct 19, 2016

Uh oh!

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

Uh oh!

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Oct 21, 2016

Uh oh!

gibfahn left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 21, 2016

Uh oh!

MylesBorins commented Nov 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

kemitchell commented Oct 18, 2016 •

edited

Loading

kemitchell commented Oct 19, 2016 •

edited

Loading