doc: improve fs.utimes #14154

refack · 2017-07-10T15:05:54Z

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

mdlint passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc,fs

refack · 2017-07-10T15:06:08Z

/cc @nodejs/documentation

vsemozhetbyt · 2017-07-10T15:12:19Z

doc/api/fs.md

+- 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.


Still "an Error"?

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.

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.

@refack Sorry, I meant a nit, still "an", not "a")

@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}

@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.

gireeshpunathil · 2017-07-10T15:48:33Z

Should the AIX >= 7.1 note be added to utimes as well?

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)

refack · 2017-07-10T16:12:18Z

Should the AIX >= 7.1 note be added to utimes as well?

No:

That's surprising since intuitively I'd assume utimes(path) := futimes(open(path)), but if no one complains 🤷‍♂️

gireeshpunathil · 2017-07-10T16:26:13Z

basically fs.utimes is implemented by uv__fs_utimes and fs.futimes by uv__fs_futime and underneath mapping to system calls of the same name in most of the unixens, but platform specificity applies. AIX cases are here and here.

BridgeAR · 2017-08-30T21:27:00Z

Where do we stand here?

BridgeAR · 2017-09-22T22:35:14Z

@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.

BridgeAR · 2017-09-28T05:17:03Z

Landed in 64e97b2

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>

refack added aix Issues and PRs related to the AIX platform. doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Jul 10, 2017

refack requested a review from bnoordhuis July 10, 2017 15:05

nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Jul 10, 2017

doc: improve fs.utimes

555d21b

refack force-pushed the doc-improve-fs-utimes branch from 9fdfe32 to 555d21b Compare July 10, 2017 15:08

refack mentioned this pull request Jul 10, 2017

fs.utimes truncate date #13520

Closed

vsemozhetbyt reviewed Jul 10, 2017

View reviewed changes

refack self-assigned this Sep 23, 2017

jasnell approved these changes Sep 25, 2017

View reviewed changes

BridgeAR approved these changes Sep 25, 2017

View reviewed changes

BridgeAR closed this Sep 28, 2017

BridgeAR pushed a commit that referenced this pull request Sep 28, 2017

doc: improve fs.utimes

64e97b2

PR-URL: #14154 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2017

doc: improve fs.utimes

ac6f957

PR-URL: nodejs#14154 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins pushed a commit that referenced this pull request Sep 29, 2017

doc: improve fs.utimes

7a04817

PR-URL: #14154 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

refack deleted the doc-improve-fs-utimes branch September 29, 2017 22:13

addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017

doc: improve fs.utimes

7b9876a

PR-URL: nodejs/node#14154 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins pushed a commit that referenced this pull request Oct 3, 2017

doc: improve fs.utimes

d93e65d

PR-URL: #14154 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

MylesBorins pushed a commit that referenced this pull request Oct 3, 2017

doc: improve fs.utimes

ec56cbe

PR-URL: #14154 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

refack removed their assignment Oct 12, 2018

Uh oh!

doc: improve fs.utimes #14154

doc: improve fs.utimes #14154

Uh oh!

Conversation

refack commented Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

refack commented Jul 10, 2017

Uh oh!

vsemozhetbyt Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

benjamingr Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Jul 10, 2017 • 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!

refack Jul 10, 2017 • 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!

refack Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

gireeshpunathil commented Jul 10, 2017

Uh oh!

refack commented Jul 10, 2017

Uh oh!

gireeshpunathil commented Jul 10, 2017

Uh oh!

BridgeAR commented Aug 30, 2017

Uh oh!

BridgeAR commented Sep 22, 2017

Uh oh!

BridgeAR commented Sep 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

refack commented Jul 10, 2017 •

edited

Loading

vsemozhetbyt Jul 10, 2017 •

edited

Loading

refack Jul 10, 2017 •

edited

Loading