doc: improved documentation for fs.unlink() #18843
Conversation
nodejs-github-bot
added
doc
doc/api/fs.md
Outdated
| @@ -2745,12 +2745,31 @@ changes: | |||
| it will emit a deprecation warning. | |||
| --> | |||
|
|
|||
| * `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! |
|
@Trott Thanks for the review! I've addressed the comments. |
|
@nodejs/documentation @nodejs/fs |
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()`][]
doc/api/fs.md
Outdated
| // 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.
|
@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.
|
@BridgeAR Wrapped! |
BridgeAR
added
the
author ready
|
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]>
Refs: #11135
Checklist
Affected core subsystem(s)
doc