doc: improved documentation for fs.unlink() by dustinnewman · Pull Request #18843 · nodejs/node

dustinnewman · 2018-02-18T00:34:59Z

Refs: #11135

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Refs: #11135

Trott · 2018-02-18T04:18:39Z

doc/api/fs.md

 -->

-* `path` {string|Buffer|URL}
+* `path` {string|Buffer|URL} File name or file descriptor


Can this really be a file descriptor? Also, "file" is not accurate, is it? This could be a symlink ~~or a directory~~, couldn't it? I think "path" is probably the best description, so I don't know that this needs a text explanation.

Trott · 2018-02-18T04:20:21Z

doc/api/fs.md

+});
+```
+
+Note that unlink() will not work on a directory, empty or otherwise.


unlink() needs backticks around it. Also, probably should be fs.unlink() for maximum clarity?

I'd also recommend dropping Note that from the start of the sentence. Just tell me what I need to know, and that I need to note it. :-D

Trott · 2018-02-18T04:24:53Z

Hi, @dustinnewman98! Thanks for the PR and welcome to the project! I left comments, and I'm sure others will too. Please don't get discouraged! Documentation changes tend to generate a lot of comments (and with good reason). Thanks again and I hope you stick with it and get this thing landed!

dustinnewman · 2018-02-18T05:33:32Z

@Trott Thanks for the review! I've addressed the comments.

Trott · 2018-02-18T05:49:18Z

@nodejs/documentation @nodejs/fs

benjamingr

Thanks! Maybe add a reference to how a directory can be deleted here instead of the negative example?

BridgeAR · 2018-02-18T10:20:05Z

doc/api/fs.md

+});
+```
+
+`fs.unlink()` will not work on a directory, empty or otherwise.


I agree with @benjamingr and think it would be good to reference the correct function in a new sentence here instead of having the example below.

dustinnewman · 2018-02-18T20:02:51Z

@BridgeAR @benjamingr Thanks for the review! I've added the suggested changes.

BridgeAR · 2018-02-18T20:19:57Z

doc/api/fs.md

+});
+```
+
+`fs.unlink()` will not work on a directory, empty or otherwise. To remove a directory, use `fs.rmdir()`.


It would be good to actually make this a link. Most functions are already listed but rmdir does not seem to be one of those. So please add the entry at the bottom of the file and write here:

[`fs.rmdir()`][]

BridgeAR · 2018-02-18T20:20:18Z

doc/api/fs.md

+  // err is not null because a directory cannot be deleted through unlink().
+});
+```
+


I actually think we do not need an error example here.

dustinnewman · 2018-02-18T20:29:11Z

@BridgeAR Made suggested changes! :)

BridgeAR · 2018-02-18T20:30:32Z

doc/api/fs.md

+});
+```
+
+`fs.unlink()` will not work on a directory, empty or otherwise. To remove a directory, use [`fs.rmdir()`][].


Oh, please wrap the line at 80 characters.

dustinnewman · 2018-02-18T20:33:50Z

@BridgeAR Wrapped!

BridgeAR

LGTM

BridgeAR · 2018-02-18T20:35:13Z

Light-CI https://ci.nodejs.org/job/node-test-commit-light/296/

mmarchini · 2018-02-20T13:08:04Z

Landed in e83adf8 😄

Refs: #11135 PR-URL: #18843 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com>

Refs: nodejs#11135 PR-URL: nodejs#18843 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com>

Refs: #11135 PR-URL: #18843 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com>

doc: improved documentation for fs.unlink()

b531ac0

Refs: #11135

nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Feb 18, 2018

Trott reviewed Feb 18, 2018

View reviewed changes

fixup! address review comments

2272df5

TimothyGu approved these changes Feb 18, 2018

View reviewed changes

benjamingr approved these changes Feb 18, 2018

View reviewed changes

BridgeAR reviewed Feb 18, 2018

View reviewed changes

fixup! Added function to remove directory.

82b486c

BridgeAR reviewed Feb 18, 2018

View reviewed changes

fixup! Addressed concerns.

8e2e47b

BridgeAR approved these changes Feb 18, 2018

View reviewed changes

BridgeAR reviewed Feb 18, 2018

View reviewed changes

fixup! wrapped at 80 characters

55f37ae

BridgeAR approved these changes Feb 18, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 18, 2018

thefourtheye approved these changes Feb 19, 2018

View reviewed changes

lpinca approved these changes Feb 19, 2018

View reviewed changes

mmarchini approved these changes Feb 20, 2018

View reviewed changes

mmarchini closed this Feb 20, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins added dont-land-on-v8.x and removed dont-land-on-v8.x labels Aug 7, 2018

MylesBorins mentioned this pull request Aug 16, 2018

v8.12.0 proposal #21593

Merged

Uh oh!

Conversation

dustinnewman commented Feb 18, 2018

Checklist

Affected core subsystem(s)

Uh oh!

Trott Feb 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott Feb 18, 2018

Choose a reason for hiding this comment

Uh oh!

Trott Feb 18, 2018

Choose a reason for hiding this comment

Uh oh!

Trott commented Feb 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dustinnewman commented Feb 18, 2018

Uh oh!

Trott commented Feb 18, 2018

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR Feb 18, 2018

Choose a reason for hiding this comment

Uh oh!

dustinnewman commented Feb 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR Feb 18, 2018

Choose a reason for hiding this comment

Uh oh!

BridgeAR Feb 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dustinnewman commented Feb 18, 2018

Uh oh!

BridgeAR Feb 18, 2018

Choose a reason for hiding this comment

Uh oh!

dustinnewman commented Feb 18, 2018

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Feb 18, 2018

Uh oh!

mmarchini commented Feb 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Trott Feb 18, 2018 •

edited

Loading

Trott commented Feb 18, 2018 •

edited

Loading

dustinnewman commented Feb 18, 2018 •

edited

Loading

BridgeAR Feb 18, 2018 •

edited

Loading