doc: improved documentation for fs.unlink()#18843
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added doc 
doc/api/fs.md Outdated
| --> | ||
| *`path`{string|Buffer|URL} | ||
| *`path`{string|Buffer|URL} File name or file descriptor |
There was a problem hiding this comment.
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.
doc/api/fs.md Outdated
| }); | ||
| ``` | ||
| Note that unlink() will not work on a directory, empty or otherwise. |
There was a problem hiding this comment.
unlink() needs backticks around it. Also, probably should be fs.unlink() for maximum clarity?
There was a problem hiding this comment.
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
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 commented Feb 18, 2018
@Trott Thanks for the review! I've addressed the comments. |
Trott commented Feb 18, 2018
@nodejs/documentation @nodejs/fs |
benjamingr left a comment
benjamingr left a comment There was a problem hiding this comment.
Thanks! Maybe add a reference to how a directory can be deleted here instead of the negative example?
doc/api/fs.md Outdated
| }); | ||
| ``` | ||
| `fs.unlink()` will not work on a directory, empty or otherwise. |
There was a problem hiding this comment.
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.
@BridgeAR@benjamingr Thanks for the review! I've added the suggested changes. |
doc/api/fs.md Outdated
| }); | ||
| ``` | ||
| `fs.unlink()` will not work on a directory, empty or otherwise. To remove a directory, use `fs.rmdir()`. |
There was a problem hiding this comment.
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()`][] | // err is not null because a directory cannot be deleted through unlink(). | ||
| }); | ||
| ``` | ||
There was a problem hiding this comment.
I actually think we do not need an error example here.
dustinnewman commented Feb 18, 2018
@BridgeAR Made suggested changes! :) |
doc/api/fs.md Outdated
| }); | ||
| ``` | ||
| `fs.unlink()` will not work on a directory, empty or otherwise. To remove a directory, use [`fs.rmdir()`][]. |
There was a problem hiding this comment.
Oh, please wrap the line at 80 characters.
dustinnewman commented Feb 18, 2018
@BridgeAR Wrapped! |
BridgeAR added the author ready 
BridgeAR commented Feb 18, 2018
mmarchini commented Feb 20, 2018
Landed in e83adf8 😄 |
Refs: #11135 PR-URL: #18843 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
Refs: #11135 PR-URL: #18843 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
Refs: #11135 PR-URL: #18843 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
Refs: #11135 PR-URL: #18843 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
Refs: nodejs#11135 PR-URL: nodejs#18843 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
Refs: #11135 PR-URL: #18843 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
Refs: #11135 PR-URL: #18843 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
Refs: #11135 PR-URL: #18843 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
Refs: #11135 PR-URL: #18843 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
10 participants
Refs: #11135
Checklist
Affected core subsystem(s)
doc