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.

Sign up for GitHub

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

Jump to bottom

doc: Clarify fs.symlink() usage #29700

Closed
wants to merge 1 commit into from
Closed

doc: Clarify fs.symlink() usage #29700

wants to merge 1 commit into from

Conversation

Granjow
Copy link
Contributor

@Granjow Granjow commented Sep 25, 2019

Clarify the fs.symlink() parameters and state that relative paths are not resolved by Node.js.

Checklist

@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 Sep 25, 2019
addaleax
addaleax approved these changes Sep 25, 2019
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
doc/STYLE_GUIDE.md Outdated Show resolved Hide resolved
@Granjow Granjow force-pushed the feature/granjow/docs-symlink branch from 1142b54 to 50e6c0b Compare September 25, 2019 21:30
Trott
Trott reviewed Sep 26, 2019
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Sep 26, 2019

@nodejs/fs

@Trott
Copy link
Member

Trott commented Sep 26, 2019 •
edited
Loading

Welcome, @Granjow! And thanks for the pull request!

@Granjow Granjow force-pushed the feature/granjow/docs-symlink branch from 50e6c0b to 5436854 Compare September 26, 2019 06:57
Trott
Trott reviewed Sep 26, 2019
View reviewed changes
doc/api/fs.md Outdated
```

It creates a symbolic link named "new-port" that points to "foo".
creates a symbolic link `mewtwo` in the `example` which points to `mew` in the
Copy link
Member

Choose a reason for hiding this comment

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

Sentence fragment?

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe the first word just needs an initial capital letter?

Copy link
Member

Choose a reason for hiding this comment

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

Oooohhh! I see what you're trying to do here. Don't use the code snippet as the subject to your sentence.

Suggested change
creates a symbolic link `mewtwo` in the `example` which points to `mew` in the
The above example creates a symbolic link `mewtwo` in the `example` which points to `mew` in the

(You'll need to re-wrap at 80 chars if you accept this suggestion.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds better indeed, changed it, thanks for the suggestion!

@Granjow Granjow force-pushed the feature/granjow/docs-symlink branch from 5436854 to 111d5f8 Compare September 27, 2019 04:48
@Trott
Copy link
Member

Trott commented Sep 27, 2019

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3941/

@Granjow Granjow force-pushed the feature/granjow/docs-symlink branch from 111d5f8 to 7ca5ce4 Compare September 28, 2019 07:27
@Granjow
Copy link
Contributor Author

Granjow commented Sep 28, 2019

Commit message fixed as well … :)

@Trott
Copy link
Member

Trott commented Sep 28, 2019

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3943/

@Granjow
Copy link
Contributor Author

Granjow commented Sep 30, 2019

@Trott CI is failing for an unrelated reason. Is it still possible to get this PR merged?

@Trott
Copy link
Member

Trott commented Sep 30, 2019

@Trott CI is failing for an unrelated reason. Is it still possible to get this PR merged?

Jenkins CI passed and that's the one that counts. Travis CI is advisory only. So yes, this can land.

@Trott
Copy link
Member

Trott commented Sep 30, 2019

Landed in 2487f39

@Trott Trott closed this Sep 30, 2019
@Trott
Copy link
Member

Trott commented Sep 30, 2019

Thanks for the contribution! 🎉

Trott pushed a commit to Trott/io.js that referenced this pull request Sep 30, 2019
@Granjow @Trott
doc: clarify fs.symlink() usage
2487f39 
PR-URL: nodejs#29700
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Granjow
Copy link
Contributor Author

Granjow commented Sep 30, 2019

Cool, thank you! =)

targos pushed a commit that referenced this pull request Oct 1, 2019
@Granjow @targos
doc: clarify fs.symlink() usage
ab060bf 
PR-URL: #29700
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Oct 1, 2019
Merged
@beevelop beevelop mentioned this pull request Oct 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott left review comments

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Granjow @Trott @jasnell @addaleax @nodejs-github-bot