doc: clarify fs.link and fs.linkSync arguments#9145
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
nodejs-github-bot added doc 
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. |
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 ( |
rvaggforce-pushed the master branch 2 times, most recently from c133999 to 83c7a88Compare
kemitchellforce-pushed the clarify-fs-link-argument-docs branch 2 times, most recently from ab688c7 to 4bbf3caCompare
ContributorAuthor
kemitchell commented Oct 19, 2016
|
ContributorAuthor
kemitchell commented Oct 19, 2016
I've also "Allow[ed] edits from maintainers." here on GitHub. |
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... |
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
Contributor
There was a problem hiding this comment.
What is the change in this line?
ContributorAuthor
kemitchell commented Oct 19, 2016 via email
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()`.
kemitchellforce-pushed the clarify-fs-link-argument-docs branch from 4bbf3ca to 5618ae2CompareContributorAuthor
kemitchell commented Oct 19, 2016
|
Updates the argument names `srcpath` and `dstpath` to match the more descriptive `existingPath` and `newPath` in the documentation.
ContributorAuthor
@Trott I pushed an additional commit changing argument names in |
thefourtheye approved these changes Oct 20, 2016
jasnell approved these changes Oct 20, 2016
Member
Trott commented 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Member
jasnell commented Oct 21, 2016
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Contributor
MylesBorins commented Nov 15, 2016
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
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.linkandfs.linkSyncargument namessrcpathanddstpathwith more descriptiveexistingpathandnewpath, reflecting how POSIX describeslink().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
oldpathandnewpath. A FreeBSD manpage I found online saysname1andname2. POSIX' listing calls thempath1andpath2. 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.