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

close #9372 add std/tempfiles #17361

Merged
merged 25 commits into from
Apr 21, 2021
Merged

close #9372 add std/tempfiles #17361

merged 25 commits into from
Apr 21, 2021

Conversation

@Varriount
Copy link
Contributor

Some general things:

  • Documentation is needed
  • Rather than sticking to C function names, descriptive names should be used.

@ringabout ringabout added the TODO: followup needed remove tag once fixed or tracked elsewhere label Mar 13, 2021
@konsumlamm
Copy link
Contributor

Does this really have to be a new module? Can't we put it in system/io or os for example? Even if so, I'd prefer a more general name (std/ioutils?), so that we can add more things in the future without having to make a new module again.

@Clyybber
Copy link
Contributor

It should be added to os where getTempDir also resides in.

@ringabout
Copy link
Member Author

ringabout commented Mar 26, 2021

std/tempfile intends to provide both low level and high level wrapper which provides auto cleaning up like Python.

TemporaryFile, NamedTemporaryFile, TemporaryDirectory, and SpooledTemporaryFile are high-level interfaces which provide automatic cleanup and can be used as context managers.

@timotheecour
Copy link
Member

timotheecour commented Mar 26, 2021

std/tempfille is expected to grow more features and deserves its own modules, std/os is too large already. Modules are a cheap abstraction. But this should be std/tempfiles, not std/tempfile
https://nim-lang.github.io/Nim/contributing.html

New module names should prefer plural form whenever possible, e.g.: std/sums.nim instead of std/sum.nim. In particular, this reduces chances of conflicts between module name and the symbols it defines.

Note that even python defines it in its own modules (https://docs.python.org/3/library/tempfile.html)

@ringabout
Copy link
Member Author

ringabout commented Apr 4, 2021

"temp" vs "stemp" suffixes for the two different classes of functions.

mktemp is deprecated since version 2.3`

I only implement the safe one(use CREATE_NEW on windows and O_EXCL on posix).

@Varriount
Copy link
Contributor

Is automatic cleanup planned?

@ringabout
Copy link
Member Author

ringabout commented Apr 13, 2021

High level API can be implemented in the future PR(by means of destructors)

already added to todo
#17361 (comment)

@ringabout
Copy link
Member Author

ringabout commented Apr 20, 2021

The PR is ready to be reviewed.

Copy link
Member

@timotheecour timotheecour left a comment

Choose a reason for hiding this comment

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

LGTM after remaining comments

@Araq
Copy link
Member

Araq commented Apr 21, 2021

Merging this now, can be further improved later.

@Araq Araq merged commit c631648 into nim-lang:devel Apr 21, 2021
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
@ringabout ringabout deleted the a152 branch October 21, 2023 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: followup needed remove tag once fixed or tracked elsewhere
Projects
None yet
Development

Successfully merging this pull request may close these issues.

We need better temporary file and dir API
8 participants