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 fs.link and fs.linkSync arguments #9145

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@kemitchell
Contributor

kemitchell commented Oct 18, 2016

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

This comment has been minimized.

Member

Trott commented Oct 18, 2016

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

This comment has been minimized.

Contributor

mscdex commented Oct 18, 2016

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... :(

@rvagg rvagg force-pushed the nodejs:master branch 2 times, most recently to 83c7a88 Oct 18, 2016

@kemitchell kemitchell force-pushed the kemitchell:clarify-fs-link-argument-docs branch 2 times, most recently Oct 19, 2016

@kemitchell

This comment has been minimized.

Contributor

kemitchell commented Oct 19, 2016

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

This comment has been minimized.

Contributor

kemitchell commented Oct 19, 2016

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

@Trott

This comment has been minimized.

Member

Trott commented Oct 19, 2016

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

This comment has been minimized.

Member

gibfahn commented Oct 19, 2016

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?

doc/api/fs.md Outdated
[Common System Errors]: errors.html#errors_common_system_errors

This comment has been minimized.

@thefourtheye

thefourtheye Oct 19, 2016

Contributor

What is the change in this line?

@kemitchell

This comment has been minimized.

Contributor

kemitchell commented Oct 19, 2016

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

@kemitchell kemitchell force-pushed the kemitchell:clarify-fs-link-argument-docs branch to 5618ae2 Oct 19, 2016

@kemitchell

This comment has been minimized.

Contributor

kemitchell commented Oct 19, 2016

  • remove newline added to end of file
fs: clarify fs.link and fs.linkSync arguments
Updates the argument names `srcpath` and `dstpath` to match the more
descriptive `existingPath` and `newPath` in the documentation.
@kemitchell

This comment has been minimized.

Contributor

kemitchell commented Oct 19, 2016

@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

lpinca approved these changes Oct 20, 2016

LGTM

@Trott

This comment has been minimized.

@gibfahn

LGTM

@jasnell jasnell self-assigned this Oct 21, 2016

jasnell added a commit that referenced this pull request Oct 21, 2016

doc: clarify fs.link and fs.linkSync arguments
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>

jasnell added a commit that referenced this pull request Oct 21, 2016

fs: clarify fs.link and fs.linkSync arguments
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

This comment has been minimized.

Member

jasnell commented Oct 21, 2016

landed in 6f7cdf7 and e8fadd8

@jasnell jasnell closed this Oct 21, 2016

@kemitchell kemitchell deleted the kemitchell:clarify-fs-link-argument-docs branch Oct 21, 2016

jasnell added a commit that referenced this pull request Oct 24, 2016

doc: clarify fs.link and fs.linkSync arguments
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>

jasnell added a commit that referenced this pull request Oct 24, 2016

fs: clarify fs.link and fs.linkSync arguments
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

This comment has been minimized.

Member

MylesBorins commented Nov 15, 2016

should this be backported? feel free to update the labels

MylesBorins added a commit that referenced this pull request Dec 21, 2016

doc: clarify fs.link and fs.linkSync arguments
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>

MylesBorins added a commit that referenced this pull request Dec 21, 2016

fs: clarify fs.link and fs.linkSync arguments
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 added a commit that referenced this pull request Dec 21, 2016

doc: clarify fs.link and fs.linkSync arguments
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>

MylesBorins added a commit that referenced this pull request Dec 21, 2016

fs: clarify fs.link and fs.linkSync arguments
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 MylesBorins referenced this pull request Dec 21, 2016

Merged

V6.9.3 proposal #10394

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment