Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
doc: make mkdtemp example work on Windows#15408
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
cjihrig left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a comment.
doc/api/fs.md Outdated
| ```js | ||
| fs.mkdtemp('/tmp/foo-', (err, folder) =>{ | ||
| consttempFolder=process.platform==='win32'?process.env['TEMP'] :'/tmp'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use os.tmpDir() here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, updated.
| if (err) throw err; | ||
| console.log(folder); | ||
| // Prints: /tmp/foo-itXde2 | ||
| // Prints: /tmp/foo-itXde2 or C:\Users\...\AppData\Local\Temp\foo-itXde2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is also incorrect for macOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be blocking though, right? As a comment it should be fine as is? (I do not have a Mac to test this on my own)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can confirm the tmp dir is different on macOS but ofc this comment should not block PR from landing.
BridgeAR commented Sep 19, 2017
Landed in 1ebde6e |
PR-URL: #15408Fixes: #14960 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #15408Fixes: #14960 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs/node#15408Fixes: nodejs/node#14960 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs/node#15408Fixes: nodejs/node#14960 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #15408Fixes: #14960 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins commented Oct 17, 2017
landed this on v6.x, please lmk if it is not relevant |
PR-URL: #15408Fixes: #14960 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Fixes: #14960
Checklist
Affected core subsystem(s)
doc