doc: Change doc on fs.rename api w.r.t to windows behaviour #22014
Conversation
nodejs-github-bot
added
doc
antsmartian
force-pushed
the
21957
branch
2 times, most recently
from
7f360eb
to
a6d171b
Compare
| @@ -2697,10 +2697,10 @@ changes: | |||
| * `callback` {Function} | |||
| * `err` {Error} | |||
|
|
|||
| Asynchronously rename file at `oldPath` to the pathname provided | |||
| as `newPath`. In the case that `newPath` already exists, it will | |||
| be overwritten. No arguments other than a possible exception are | |||
There was a problem hiding this comment.
Is the deletion of the last sentence intended?
doc/api/fs.md
Outdated
| given to the completion callback. | ||
| Asynchronously rename file or directory at `oldPath` to the pathname provided | ||
| as `newPath`. In the case that `newPath` is a file and already exists, it will | ||
| be overwritten. If there is a directory at `newPath` an error will be raised instead. |
There was a problem hiding this comment.
Linter nit: this line needs to be wrapped to not exceed 80 characters.
There was a problem hiding this comment.
As described in ##21957 (comment) that is actually not always true. Unix systems will not raise an exception if the directory is empty. That is only the case for Windows. I did not check but that should hold true for all OS.
|
Is this true for all supported systems? |
|
Hi @vsemozhetbyt , actually I think the issue was that the result differs from system to system so I think It wold be nice if the docs inform you about each case, although I don't know if it's possible. |
|
I thought of writing like: Asynchronously rename file or directory at
No arguments other than a possible exception (like mentioned above) are given to Thoughts? cc @vsemozhetbyt @BridgeAR |
|
ping @vsemozhetbyt :) |
| given to the completion callback. | ||
| be overwritten. `fs.rename` can throw the below error: | ||
|
|
||
| - On **Linux** when renaming directory exists and is NOT empty, it throws a |
There was a problem hiding this comment.
- Not sure if
renamingis a right form here and below. Let's see what native speakers think. a `ENOTEMPTY`- >an `ENOTEMPTY`(as well as below).
There was a problem hiding this comment.
Its not clear or grammatical, call it after the name of the arg: "if newPath exists"... and likewise in other places.
|
Unfortunately, I have not enough knowledge in this realm to fully review. Maybe @nodejs/fs can assess? |
|
To be honest I feel like this inconsistency is a bug that we need to fix rather than document.. |
|
What's the status on this one? |
|
@jasnell I guess doc is fine (until bug fix), but @joyeecheung feels we need to fix. So need input from them. |
|
@joyeecheung If there is any plan to fix the actual issue? (Since we discussed documentation wouldn't help much here) |
|
If I am not wrong, this has to be addressed at libuv level. I'll take a look at it and update within this week. |
| - On **Windows** regardless of renaming directory is empty, it throws a | ||
| `EPERM` error. | ||
|
|
||
| No arguments other than a possible exception (as mentioned above) are given to |
There was a problem hiding this comment.
This just repeats line 2698 in a more complex written form, and it adds confusion. It says an exception is given to the completion callback, but it is an error that is given. Also, more errors than the few documented can occur, and the sentence reads to me as saying that only the errors documented above can be passed. Better, I think, to remove the sentence.
There was a problem hiding this comment.
@sam-github Make sense.
There was a PR in libuv/libuv#1941, but it didn't really gain any traction. |
Awesome we already have some code then. Let's dust it up and reuse 😊 |
|
Looks like this can be closed. Please re-open if needed. |
Fixes : #21957
make -j4 test(UNIX), orvcbuild test(Windows) passes