Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: improve fs.utimes #14154

Closed
wants to merge 1 commit into from
Closed

Conversation

refack
Copy link
Contributor

@refack refack commented Jul 10, 2017

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 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 refack requested a review from bnoordhuis July 10, 2017 15:05
@nodejs-github-bot 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
@refack
Copy link
Contributor Author

refack commented Jul 10, 2017

/cc @nodejs/documentation

- 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still "an Error"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Still an Error in master - https://github.com/nodejs/node/blob/master/lib/fs.js#L1196
  2. 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.

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@refack refack Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Member

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
Copy link
Contributor Author

refack commented Jul 10, 2017

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
Copy link
Member

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
Copy link
Member

Where do we stand here?

@BridgeAR
Copy link
Member

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

@refack refack self-assigned this Sep 23, 2017
@BridgeAR
Copy link
Member

Landed in 64e97b2

@BridgeAR BridgeAR closed this Sep 28, 2017
BridgeAR pushed a commit that referenced this pull request Sep 28, 2017
PR-URL: #14154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2017
PR-URL: nodejs#14154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
PR-URL: #14154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@refack 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
PR-URL: nodejs/node#14154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
PR-URL: #14154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
PR-URL: #14154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@refack refack removed their assignment Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants