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.

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

Conversation

kemitchell
Copy link
Contributor

@kemitchell 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.

@nodejs-github-bot 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
@Trott
Copy link
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
Copy link
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 master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
@kemitchell kemitchell force-pushed the clarify-fs-link-argument-docs branch 2 times, most recently from ab688c7 to 4bbf3ca Compare October 19, 2016 18:40
@kemitchell
Copy link
Contributor Author

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

@kemitchell
Copy link
Contributor Author

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

@Trott
Copy link
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
Copy link
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?

[Common System Errors]: errors.html#errors_common_system_errors
Copy link
Contributor

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?

@kemitchell
Copy link
Contributor Author

kemitchell commented Oct 19, 2016 via email

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

  • 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
Copy link
Contributor Author

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.

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member

Trott commented Oct 21, 2016

Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell jasnell self-assigned this Oct 21, 2016
jasnell pushed a commit that referenced this pull request Oct 21, 2016
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 pushed a commit that referenced this pull request Oct 21, 2016
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
Copy link
Member

jasnell commented Oct 21, 2016

landed in 6f7cdf7 and e8fadd8

@jasnell jasnell closed this Oct 21, 2016
@kemitchell kemitchell deleted the clarify-fs-link-argument-docs branch October 21, 2016 15:53
jasnell pushed a commit that referenced this pull request Oct 24, 2016
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 pushed a commit that referenced this pull request Oct 24, 2016
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
Copy link
Contributor

should this be backported? feel free to update the labels

MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
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 pushed a commit that referenced this pull request Dec 21, 2016
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 pushed a commit that referenced this pull request Dec 21, 2016
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 pushed a commit that referenced this pull request Dec 21, 2016
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 mentioned this pull request Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants