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

Clarify docs for ensureSymlink() when destPath already exists #869

Closed
wants to merge 1 commit into from

Conversation

mwest1066
Copy link

The existing docs for ensureSymlink() don't specify what happens in the case where destPath already exists. Looking at the code it turns out that when destFile already exists the function exits without making any modifications, whether or not destPath is a symlink to srcPath:

if (destinationExists) return callback(null)

I found this behavior surprising, because the function name ensureSymlink() sounds like it will ensure that the symlink destPath -> srcPath exists. This PR clarifies the existing behavior in the docs.

…xists

The existing docs for `ensureSymlink()` don't specify what happens in the case where `destPath` already exists. Looking at the code it turns out that when `destFile` already exists the function exits without making any modifications, whether or not `destPath` is a symlink to `srcPath`: https://github.com/jprichardson/node-fs-extra/blob/1625838cdfc65a1bbf28ab5fa962a75805629b9c/lib/ensure/symlink.js#L26

I found this behavior surprising, because the function name `ensureSymlink()` sounds like it will ensure that the symlink `destPath -> srcPath` exists. This PR clarifies the existing behavior in the docs.
@mwest1066
Copy link
Author

Sorry! I just realized that #786 and #826 already address this issue.

I would love to see #826 merged, but until it is, it would be great to clarify the current behavior in the docs.

@RyanZim
Copy link
Collaborator

RyanZim commented Apr 1, 2021

Gonna try to land #826 soon, so closing this.

@RyanZim RyanZim closed this Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants