Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

fs: fix link and symlink error messages #9263

Closed
wants to merge 2 commits into from
Closed

fs: fix link and symlink error messages #9263

wants to merge 2 commits into from

Conversation

schnittstabil
Copy link

Currently, the error messages for missing arguments of symlink, link, symlinkSync and linkSync are swapped. Additionally they are named inconsistently all over the sources and the docs.

From the docs:

fs.link(srcpath, dstpath, callback)
fs.linkSync(srcpath, dstpath)
fs.symlink(srcpath, dstpath, callback)
fs.symlinkSync(srcpath, dstpath)

But the error messages doesn't match:

var fs = require('fs');
> fs.symlink('srcpath')
TypeError: src path must be a string

There are two things to mention here:

  1. dstpath is missing, not the srcpath argument
  2. it's not src, it's srcpath

In addition, #8920 shows that the meaning of srcpath and dstpath isn't clear and the docs aren't comprehensive enough.

This PR changes srcpath to target and dstpath to path in the docs and the sources, and make the code more consistent.

Currently, the error messages for missing arguments of symlink, link,
symlinkSync and linkSync are swapped. Additionally they are named
inconsistently all over the sources and the docs.

In addition, #8920 shows that the meaning of `srcpath` and `dstpath`
isn't clear and the docs aren't comprehensive enough.

This PR changes `srcpath` to `target` and `dstpath` to `path` in the docs and
the sources and make the code more consistent.
@piscisaureus
Copy link

In io.js this was further improved: nodejs/node@bc2c85c

@jasnell
Copy link
Member

jasnell commented Mar 4, 2015

@piscisaureus does the io.js patch include the commits in PR, complement them, or provide an alternate for them?

@piscisaureus
Copy link

@jasnell my patch predates this one. It fixes more things, including an incorrect invocation of ThrowUVException(). Look at the commit message to see all the details. And feel free to judge for yourself.

We may want to land the test and doc changes from this PR.

@schnittstabil
Copy link
Author

I like the improvements of @piscisaureus, but haven't known them.

The only thing that I personally would prefer are better parameter names for link and symlink, consistently used within sources and docs, so that error messages aren't such misleading (see also here).

From ln:

ln [OPTION]... [-T] TARGET LINK_NAME
ln [OPTION]... TARGET... DIRECTORY

From mklink:

mklink [[/d] | [/h] | [/j]] <Link> <Target> // swapped order regarding ln

Therefore I'd vote for target and [link]path - instead of src/dest and destination/path.

@jasnell
Copy link
Member

jasnell commented Aug 16, 2015

@schnittstabil ... if you'd like to pursue this change further, opening a new updated one against http://github.com/nodejs/node master would be the right approach. This is not going to be able to land here in it's current state. I'm going to go ahead and close this but can reopen if necessary

@jasnell jasnell closed this Aug 16, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants