doc: improve fs.utimes - #14154
Conversation
Sorry, something went wrong.
|
/cc @nodejs/documentation |
Sorry, something went wrong.
| - Values can be either numbers representing Unix epoch time, `Date`s, or a | ||
| numeric string like `'123456789.0'`. | ||
| - If the value can not be converted to a number, or is `NaN`, `Infinity` or | ||
| `-Infinity`, a `Error` will be thrown. |
There was a problem hiding this comment.
Still "an Error"?
Sorry, something went wrong.
There was a problem hiding this comment.
What happens if Invalid Date is passed?
Also, if we can pass a string that gets coerced to numbers, then I'm not sure we should put string in the docs.
Sorry, something went wrong.
There was a problem hiding this comment.
- Still an Error in master - https://github.com/nodejs/node/blob/master/lib/fs.js#L1196
- Even a Date is converted (or coerced) to number so the sentence still stands true (Number(new Date("foo")) === NaN), just missing an explicit mention.
Sorry, something went wrong.
There was a problem hiding this comment.
@refack Sorry, I meant a nit, still "an", not "a")
Sorry, something went wrong.
There was a problem hiding this comment.
@benjamingr the string note was alway there. As for the arg type I think it's better to explicitly state string but this is JS1.0 semantics so we need to ask ourselves WWDCD?
What Would Douglas Crockford Do
Sorry, something went wrong.
There was a problem hiding this comment.
@refack Sorry, I meant a nit, still "an", not "a")
Ack. But I'm not pushing a fix yet, so the string conversation will stay open.
Sorry, something went wrong.
No: bash-4.3$ uname -a
AIX 1 6 00F460A94C00
bash-4.3$ touch g.txt
bash-4.3$ cat utimes.js
var fs = require('fs')
fs.utimes(process.argv[2], 1, 2, () => {
const stat = fs.statSync(process.argv[2])
console.log(`file modified at ${stat.mtime} and accessed at ${stat.atime}`)
})bash-4.3$ ./node utimes.js g.txt file modified at Thu Jan 01 1970 00:00:02 GMT+0000 (EST) and accessed at Thu Jan 01 1970 00:00:01 GMT+0000 (EST) |
Sorry, something went wrong.
That's surprising since intuitively I'd assume utimes(path) := futimes(open(path)), but if no one complains 🤷♂️ |
Sorry, something went wrong.
|
Where do we stand here? |
Sorry, something went wrong.
|
@refack as far as I see it there is little to do to get this ready to land, right? I would otherwise close the PR sometime soon. |
Sorry, something went wrong.
PR-URL: #14154 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: nodejs#14154 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #14154 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: nodejs/node#14154 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #14154 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #14154 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
AFAICT the internal conversion helper toUnixTimestamp has always been able to convert Dates - https://github.com/nodejs/node/commit/1d5ff15
Currently precision is platform dependant, but that could be fixed.
@nodejs/platform-aix - Should the AIX >= 7.1 note be added to utimes as well? #14154 (comment)
/cc @bnoordhuis @nodejs/fs
Checklist
Affected core subsystem(s)
doc,fs