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

Add the mkdtemp and mkstemp methods to fs #5332

Closed
ralt opened this issue Feb 19, 2016 · 10 comments
Closed

Add the mkdtemp and mkstemp methods to fs #5332

ralt opened this issue Feb 19, 2016 · 10 comments
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system.

Comments

@ralt
Copy link
Contributor

ralt commented Feb 19, 2016

I would like to propose the addition of mkdtemp and mkstemp to the fs module, to create a temporary directory and file, respectively.

Some manual pages to start with:

So, this utility is fairly often needed for various reasons. Not providing it in nodejs itself means that people try to rewrite it, badly. (Or may I say, inefficiently.) (I'm obviously not trying to put the blame on someone there, except on node.js for not providing the functions ;-).)

This is part of the standard library in most other languages. Whether it's C, python, java, and surely others. I believe node.js would benefit its addition as well.

@ChALkeR ChALkeR added fs Issues and PRs related to the fs subsystem / file system. feature request Issues that request new features to be added to Node.js. labels Feb 19, 2016
@ralt
Copy link
Contributor Author

ralt commented Feb 19, 2016

I have opened a pull request implementing fs.mkdtemp, libuv already providing it. Adding fs.mkstemp would require adding it in libuv first, so I guess this part of the proposal should be postponed.

@jasnell
Copy link
Member

jasnell commented Feb 20, 2016

/cc @nodejs/ctc ... overall I don't have a problem with this but aren't there userland modules that already cover this?

@ralt
Copy link
Contributor Author

ralt commented Feb 20, 2016

@jasnell in fact, I linked a userland version of this, but it does so:

  • In a non de-facto standard way
  • Highly inefficiently

As also pointed out, this function is part of the standard library of many other languages for its utility and because it's easy to mess it up. (See the python page about this.)

@silverwind
Copy link
Contributor

I think it's a worthfile feature to have. As for the API, I'd suggest following Linux's command scheme and only provide a single fs.mktemp, with options for the variants or else we might end up with a lot of new methods. Also I think we'd want a fs.mktempSync.

It's a bit unfortunate that plain mktemp is not in libuv yet, maybe wait for it so we have a single, clean API?

cc: @saghul

@saghul
Copy link
Member

saghul commented Feb 20, 2016

I wouldn't oppose adding this to libuv.
On Feb 20, 2016 12:22, "silverwind" notifications@github.com wrote:

I think it's a worthfile feature to have. As for the API, I'd suggest
following Linux's scheme and only provide a single fs.mktemp, with
options for the variants. Also I think we'd want a fs.mktempSync. It's a
bit unfortunate that mktemp is not in libuv yet, maybe wait for it so we
have a single, clean API?

cc: @saghul https://github.com/saghul


Reply to this email directly or view it on GitHub
#5332 (comment).

@jasnell
Copy link
Member

jasnell commented Feb 20, 2016

That's good enough for me! I just want to make sure we're being deliberate
and careful about which pieces of userland functionality we're replacing or
duplicating.
On Feb 20, 2016 2:05 AM, "Florian Margaine" notifications@github.com
wrote:

@jasnell https://github.com/jasnell in fact, I linked a userland
version of this, but it does so:

  • In a non de-facto standard way
  • Highly inefficiently

As also pointed out, this function is part of the standard library of many
other languages for its utility and because it's easy to mess it up. (See
the python page about this.)


Reply to this email directly or view it on GitHub
#5332 (comment).

@ralt
Copy link
Contributor Author

ralt commented Feb 20, 2016

@silverwind I don't think we should have fs.mktemp, for several reasons:

  • Having a single function with a flag argument is usually a code smell. (I guess that's debatable.)
  • Only the CLI provides mktemp, all the programming APIs provide mkdtemp and mkstemp.

@silverwind
Copy link
Contributor

@ralt let's say we implement all variants, how many methods are we looking at?

@ralt
Copy link
Contributor Author

ralt commented Feb 21, 2016

@silverwind 2. libc provides a couple more, but all the other APIs only provide mkdtemp and mkstemp.

@ralt
Copy link
Contributor Author

ralt commented Apr 23, 2016

Closing this since fs.mkdtemp was added in nodejs 5.10.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

No branches or pull requests

5 participants