doc,fs: keep consistent with fs.link and fs.linkSync by yorkie · Pull Request #3912 · nodejs/node

yorkie · 2015-11-19T10:14:53Z

So firstly, the destination has a little confusion like I got the log:

Error: EEXIST: file already exists, symlink 'dist/node_modules' -> 'node_modules'

And my source call is:

fs.symlinkSync('dist/node_modules','node_modules','dir');

Because I thought the first argument stands for destination as the original doc is telling, but it actually is source. Plus, I also make link* and symlinkSync be consistent :-)

targos · 2015-11-19T11:23:53Z

In man ln the arguments are called TARGET and LINK_NAME. Perhaps we should be consistent with that and pick target and linkName or just name instead ?

evanlucas · 2015-11-19T12:48:26Z

man ln will be different across platforms though. On OS X, it is source_file and target_file (or target_dir)

yorkie · 2015-11-19T13:05:45Z

Ah, I didn't know it's consistent with man command. BTW, if as @evanlucas said, this behaviors are different across platforms, shouldn't the cross-platform Node.js unify it?

(I'm using OSX).

yorkie · 2015-11-19T13:55:50Z

Oh, @targos @evanlucas Why do you reference the command ln, what I have changed on is symlink(2), am I right? After reading at the man symlink, which uses path1 and path2 as below:

A symbolic link path2 is created to path1 (path2 is the name of the file cre-
ated, path1 is the string used in creating the symbolic link). Either name
may be an arbitrary path name; the files need not be on the same file system.

Then now I'm able to understand why the author of this function used destination as the name of 1st one, because the context said there is a pointer to the file described by the 1st one, that we called it source at other function like fs.link.

However, not all Node.js user is such that familiar with Linux like me, then the naming of arguments in fs.link and fs.symlink were really really confused. So I still proposal that keep consistent with fs.link's naming rule, any thoughts?

evanlucas · 2015-11-19T13:59:14Z

Ahhh good point. I think that this is less confusing than the current documentation.

LGTM

targos · 2015-11-19T14:01:11Z

You also need to update mentions of "destination path" here.

targos · 2015-11-19T14:03:10Z

Fair enough, I didn't get that the point was to make it consistent with fs.link, sorry !

yorkie · 2015-11-19T14:21:43Z

Never mind @targos, fixed nits and updated the naming in source files, LGTY too?

targos · 2015-11-19T14:28:38Z

LGTM

CI: https://ci.nodejs.org/job/node-test-commit/1183/

yorkie · 2015-11-19T14:29:45Z

BTW, I found a typo of errors in fs.link and created a new PR at #3917 :-)

jasnell · 2015-11-19T17:31:03Z

@yorkie ... awesome. This LGTM but curious what @nodejs/ctc would think about the semver-iness of this change. It seems like a fairly significant mismatch between the docs, the API and the implementation in JS.

Also, minor nit, can you please update the title of the PR to be doc,fs: to reflect that it's a change to both the doc and the fs module?

jasnell · 2015-11-19T17:32:02Z

I'm going to mark this semver-major for now as a precaution.

yorkie · 2015-11-19T17:34:33Z

Done :-)

yorkie · 2015-11-23T14:50:49Z

How is this going? is the CI happy now? This seems to be suspended over 4 days :-)

trevnorris · 2015-11-23T20:43:32Z

Just to be clear, this change won't break any existing code or will it?

piscisaureus · 2015-11-23T22:15:45Z

The documentation change is incorrect. I reverted a similar change in in 5843ae8.

piscisaureus · 2015-11-23T22:24:23Z

Let me clarify a bit, too.

symlinkSync(destination, path) creates a symbolic link at path which points at destination.

If this patch gets landed, it becomes this:

symlinkSync(srcpath, dstpath) creates a symbolic link at dstpath which points at srcpath.
I find that wording very confusing, I (and I think most people) would assume the source to point at the destination, and not the other way round.

If the fs.link() documentation is unclear, I would prefer to see that fixed rather than applied to other documentation sections.

yorkie · 2015-11-24T06:34:19Z

Thanks @piscisaureus and now I have some explanations more about this question, the following is a description from man link/ln:

Given one or two arguments, ln creates a link to an existing file source_file.

The above manual seems to use source to stand for the existing file, and the below is also from manual:

How a link points to a file is one of the differences between a hard and symbolic link.

So I think the file pointed from the link should be unified by this manual.

creates a symbolic link at dstpath which points at srcpath

@piscisaureus as you said, I correct your words as: creates a symbolic link/pointer to the source_file, and this link/pointer is named by target_file, so I'm proposing that Node.js should use target instead of dest in link, symlink.

targos · 2015-11-24T08:19:18Z

creates a symbolic link/pointer to the source_file, and this link/pointer is named by target_file

It's not that easy. In the Linux doc, target is used for the path that the symlink points at: http://man7.org/linux/man-pages/man2/symlink.2.html

piscisaureus · 2015-11-24T14:42:09Z

@piscisaureus as you said, I correct your words as: creates a symbolic link/pointer to the source_file, and this link/pointer is named by target_file, so I'm proposing that Node.js should use target instead of dest in link, symlink.

A symbolic link points to another path. There doesn't actually need to be a file there. I think "target" is an appropriate name for the path that the symbolic link points to.

link() is different in the sense that it doesn't point at another path - it adds a new name for a file that already exists. It has more in common with rename(old_name, new_name) than it has with symlink - the difference being that linkadds a name and renamechanges the name. So I would suggest the link docs be made consistent with rename rather than symlink.

You can see the linux man pages do the same thing:

versus

int symlink(const char *target, const char *linkpath)

yorkie · 2015-11-25T04:43:34Z

@piscisaureus I do that agree with you 100%, and I now updated my commits to use target instead of destination. Could you take a look again, thank you guys :-)

piscisaureus · 2015-11-25T14:44:21Z

LGTM. Thanks for your patience @yorkie!

evanlucas · 2015-11-25T14:45:43Z

LGTM if CI is happy.

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

evanlucas · 2015-11-25T14:49:57Z

@yorkie CI doesn't seem to be happy with this change.

https://ci.nodejs.org/job/node-test-commit-osx/1298/nodes=osx1010/console

yorkie · 2015-11-25T14:51:17Z

Sorry, I guess I have something forgot to update, I will check it again, sorry :-)

targos · 2015-12-01T16:53:04Z

That's right

jasnell · 2015-12-01T16:53:10Z

@Trott +1... label removed

jasnell · 2015-12-01T16:53:33Z

LGTM
(sorry for the delay reviewing @yorkie , just catching up after vacation ;-) )

targos · 2015-12-01T16:53:44Z

LGTM

yorkie · 2015-12-01T16:54:48Z

Btw, if semver is right to marjor, when this would get this merged? Does this get documented?

yorkie · 2015-12-01T16:55:02Z

Thank you guys :-)

yorkie · 2015-12-05T19:25:41Z

Ping guys

Trott · 2015-12-05T23:51:26Z

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

yorkie · 2015-12-06T01:36:30Z

@Trott Why create a new CI? because I didn't push any changes to the last CI.

Trott · 2015-12-06T01:40:47Z

Whoops, my mistake.

targos · 2015-12-06T01:42:28Z

@yorkie your last push was 10 days ago, it can be possible that new commits on master make the changes in this PR fail

Trott · 2015-12-06T01:45:35Z

FWIW, the latest CI passed except for known-flaky tests on Windows (including one that would be fixed by #4062, hey someone go look at that and give it an LGTM if you're comfy with the change, will ya?).

PR-URL: #3912 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Bert Belder <bertbelder@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Trott · 2015-12-06T02:28:47Z

Landed in 8c35903

(The patch didn't apply cleanly on fs.markdown so check my work carefully on that one to make sure I didn't mess anything up.)

yorkie · 2015-12-06T05:53:09Z

@Trott I guess you missed to add @trevnorris to reviewers list at 8c35903

yorkie · 2015-12-06T05:54:38Z

Never mind, that's my fault, thank you Trott :-)

PR-URL: #3912 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Bert Belder <bertbelder@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: nodejs#3912 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Bert Belder <bertbelder@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

targos added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Nov 19, 2015

targos reviewed Nov 19, 2015
View reviewed changes

Comment threaddoc/api/fs.markdown Outdated
targosNov 19, 2015
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need to update mentions of "destination path" here.

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 19, 2015

yorkie changed the title ~~doc: keep consistent with fs.link and fs.linkSync~~doc,fs: keep consistent with fs.link and fs.linkSyncNov 19, 2015

yorkie force-pushed the improve/fs-doc branch from d3bfaa1 to e320969Compare November 25, 2015 04:42

jasnell removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 1, 2015

Trott closed this Dec 6, 2015

yorkie deleted the improve/fs-doc branch December 6, 2015 05:51

This was referenced Dec 8, 2015
Release proposal: v5.2.0 #4180
Closed
Release proposal: v5.2.0 (Stable) #4181
Merged

rvagg mentioned this pull request Dec 17, 2015
Release 4.2.4 Planning #4321
Closed

jasnell added the lts-watch-v4.x label Dec 17, 2015

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Dec 29, 2015

MylesBorins mentioned this pull request Jan 19, 2016
V4.2.5 proposal #4768
Merged

Uh oh!

Uh oh!

Conversation

yorkie commented Nov 19, 2015

Uh oh!

targos commented Nov 19, 2015

Uh oh!

evanlucas commented Nov 19, 2015

Uh oh!

yorkie commented Nov 19, 2015

Uh oh!

yorkie commented Nov 19, 2015

Uh oh!

evanlucas commented Nov 19, 2015

Uh oh!

targosNov 19, 2015

Choose a reason for hiding this comment

Uh oh!

targos commented Nov 19, 2015

Uh oh!

yorkie commented Nov 19, 2015

Uh oh!

targos commented Nov 19, 2015

Uh oh!

yorkie commented Nov 19, 2015

Uh oh!

jasnell commented Nov 19, 2015

Uh oh!

jasnell commented Nov 19, 2015

Uh oh!

yorkie commented Nov 19, 2015

Uh oh!

yorkie commented Nov 23, 2015

Uh oh!

trevnorris commented Nov 23, 2015

Uh oh!

piscisaureus commented Nov 23, 2015

Uh oh!

piscisaureus commented Nov 23, 2015

Uh oh!

yorkie commented Nov 24, 2015

Uh oh!

targos commented Nov 24, 2015

Uh oh!

piscisaureus commented Nov 24, 2015

Uh oh!

yorkie commented Nov 25, 2015

Uh oh!

piscisaureus commented Nov 25, 2015

Uh oh!

evanlucas commented Nov 25, 2015

Uh oh!

evanlucas commented Nov 25, 2015

Uh oh!

yorkie commented Nov 25, 2015

Uh oh!

targos commented Dec 1, 2015

Uh oh!

jasnell commented Dec 1, 2015

Uh oh!

jasnell commented Dec 1, 2015

Uh oh!

targos commented Dec 1, 2015

Uh oh!

yorkie commented Dec 1, 2015

Uh oh!

yorkie commented Dec 1, 2015

Uh oh!

yorkie commented Dec 5, 2015

Uh oh!

Trott commented Dec 5, 2015

Uh oh!

yorkie commented Dec 6, 2015

Uh oh!

Trott commented Dec 6, 2015

Uh oh!

targos commented Dec 6, 2015

Uh oh!

Trott commented Dec 6, 2015

Uh oh!