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

fs.truncate accepts an fd but it's poorly documented #15753

Closed
seishun opened this issue Oct 3, 2017 · 3 comments
Closed

fs.truncate accepts an fd but it's poorly documented #15753

seishun opened this issue Oct 3, 2017 · 3 comments
Labels
fs Issues and PRs related to the fs subsystem / file system. good first issue Issues that are suitable for first-time contributors.

Comments

@seishun
Copy link
Contributor

seishun commented Oct 3, 2017

  • Version: master
  • Platform: all
  • Subsystem: fs

As I just found out from #15409, you can call fs.truncate with an fd:

> var fd = fs.openSync('asdf.txt', 'r+')
undefined
> fs.truncateSync(fd)
undefined

But this behavior is surprising, since there's fs.ftruncate for fds and it's easy to overlook the note in the documentation for fs.truncate.

Options:

  1. Improve documentation for fs.truncate to make it more obvious that it also takes fds. For instance, path <string> | <Buffer> needs to be replaced with something like pathOrFd <string> | <Buffer> | <number>, and Asynchronous truncate(2). needs to be replaced with something like Asynchronous truncate(2) or ftruncate(2) depending on the argument.
  2. Phase out fd support in fs.truncate. This would be my preference, since it would avoid having two functions that can do the same thing, and it would make code such as fs.truncate(f) more readable because it would be obvious that f is a path and not an fd.
@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Oct 3, 2017
@Fishrock123 Fishrock123 added the good first issue Issues that are suitable for first-time contributors. label Oct 5, 2017
@ghost
Copy link

ghost commented Oct 6, 2017

@Fishrock123 @mscdex @seishun I'm for option 2.

  1. I'll print a deprecation warning in
    if (typeof path === 'number') { return fs.ftruncate(path, len, callback); }

  2. Remove from docs in ftruncate,
    A file descriptor can also be passed as the first argument. In this case, fs.ftruncate() is called.

  3. Add a test

Is this the correct approach?

@r1cebank
Copy link

r1cebank commented Oct 6, 2017

@Fishrock123 @mscdex I will start work on this in favor of deprecation

@benjamingr
Copy link
Member

Closed with #15990

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

No branches or pull requests

5 participants