doc: improve fs.truncate functions' documentation #7648

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants
@thefourtheye
Contributor

thefourtheye commented Jul 10, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc, fs

Description of change

The default value of the len parameter is zero and it is included in
the documentation.

This patch also has an example of how ftruncate can be used.

@nodejs/fs @nodejs/documentation

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@addaleax

View changes

doc/api/fs.md
* `callback` {Function}
Asynchronous ftruncate(2). No arguments other than a possible exception are
given to the completion callback.
+If the file referred by the file descriptor has more number of bytes than the
+`len` then, only the first `len` bytes will be retained in the file.

This comment has been minimized.

@addaleax

addaleax Jul 11, 2016

Member

From ftruncate(2):

The truncate() and ftruncate() functions cause the regular file named by path or referenced by fd to be truncated to a size of precisely length bytes.

If the file previously was larger than this size, the extra data is lost. If the file previously was shorter, it is extended, and the extended part reads as null bytes ('\0').

I think something along these lines might be nice?

@addaleax

addaleax Jul 11, 2016

Member

From ftruncate(2):

The truncate() and ftruncate() functions cause the regular file named by path or referenced by fd to be truncated to a size of precisely length bytes.

If the file previously was larger than this size, the extra data is lost. If the file previously was shorter, it is extended, and the extended part reads as null bytes ('\0').

I think something along these lines might be nice?

This comment has been minimized.

@thefourtheye

thefourtheye Jul 13, 2016

Contributor

I don't know how to write this better. Suggestions?

@thefourtheye

thefourtheye Jul 13, 2016

Contributor

I don't know how to write this better. Suggestions?

This comment has been minimized.

@addaleax

addaleax Jul 13, 2016

Member
If the file referred to by the file descriptor was larger than `len` bytes, only the first `len` bytes will be retained in the file.

How about that (it’s only a minor difference to what you wrote)?

The second paragraph below could probably be written quite closely to the man page, e.g.

If the file previously was shorter than `len` bytes, it is extended, and the extended part reads as null bytes ('\0').
@addaleax

addaleax Jul 13, 2016

Member
If the file referred to by the file descriptor was larger than `len` bytes, only the first `len` bytes will be retained in the file.

How about that (it’s only a minor difference to what you wrote)?

The second paragraph below could probably be written quite closely to the man page, e.g.

If the file previously was shorter than `len` bytes, it is extended, and the extended part reads as null bytes ('\0').
@addaleax

View changes

doc/api/fs.md
+ });
+undefined
+> Node
+```

This comment has been minimized.

@addaleax

addaleax Jul 11, 2016

Member

````js`?

I also think it might be nicer to have this as a script that could be run directly rather than a REPL excerpt, but that’s obviously a question of personal taste.

@addaleax

addaleax Jul 11, 2016

Member

````js`?

I also think it might be nicer to have this as a script that could be run directly rather than a REPL excerpt, but that’s obviously a question of personal taste.

This comment has been minimized.

@trevnorris

trevnorris Jul 11, 2016

Contributor

heh. what I get for leaving my comment sitting around for too long before actually submitting it.

@trevnorris

trevnorris Jul 11, 2016

Contributor

heh. what I get for leaving my comment sitting around for too long before actually submitting it.

@addaleax

View changes

test/parallel/test-fs-truncate.js
+ fs.writeFileSync(filename1, 'Hi');
+ fs.truncateSync(filename1, 4);
+ assert.strictEqual(
+ fs.readFileSync(filename1).compare(Buffer.from('Hi\u0000\u0000')), 0

This comment has been minimized.

@addaleax

addaleax Jul 11, 2016

Member

Using assert.ok + .equals instead of .compare should get this under 80 characters

@addaleax

addaleax Jul 11, 2016

Member

Using assert.ok + .equals instead of .compare should get this under 80 characters

This comment has been minimized.

@thefourtheye

thefourtheye Jul 13, 2016

Contributor

Done!

@thefourtheye

thefourtheye Jul 13, 2016

Contributor

Done!

@addaleax

View changes

test/parallel/test-fs-truncate.js
+{
+ fs.writeFileSync(filename1, 'Hi');
+ const fd = fs.openSync(filename1, 'r+');
+ process.on('exit', () => fs.closeSync(fd));

This comment has been minimized.

@addaleax

addaleax Jul 11, 2016

Member

Is the process.on('exit') for the case that the assertion fails?

@addaleax

addaleax Jul 11, 2016

Member

Is the process.on('exit') for the case that the assertion fails?

This comment has been minimized.

@thefourtheye

thefourtheye Jul 13, 2016

Contributor

I was under the assumption that the fds will not be closed automatically if the process errors out. That is why I manually clean the fds.

@thefourtheye

thefourtheye Jul 13, 2016

Contributor

I was under the assumption that the fds will not be closed automatically if the process errors out. That is why I manually clean the fds.

This comment has been minimized.

@addaleax

addaleax Jul 13, 2016

Member

fds will be implicitly closed by the OS when the process exits, so you don’t have to worry about that here.

(edit: It’s not a bad thing to close them, either… that’s actually good practice. That was more of a general question, not something that needs to be fixed).

@addaleax

addaleax Jul 13, 2016

Member

fds will be implicitly closed by the OS when the process exits, so you don’t have to worry about that here.

(edit: It’s not a bad thing to close them, either… that’s actually good practice. That was more of a general question, not something that needs to be fixed).

@addaleax

View changes

test/parallel/test-fs-truncate.js
+ const filename2 = path.resolve(tmp, 'truncate-file-2.txt');
+ fs.writeFileSync(filename2, 'Hi');
+ fs.truncate(filename2, 3, common.mustCall(function(err) {
+ assert(!err);

This comment has been minimized.

@addaleax

addaleax Jul 11, 2016

Member

assert.ifError(err);

@addaleax

addaleax Jul 11, 2016

Member

assert.ifError(err);

This comment has been minimized.

@thefourtheye

thefourtheye Jul 13, 2016

Contributor

Done!

@thefourtheye

thefourtheye Jul 13, 2016

Contributor

Done!

@trevnorris

View changes

doc/api/fs.md
+
+For example, the following program retains only the first four bytes of the file
+
+```

This comment has been minimized.

@trevnorris

trevnorris Jul 11, 2016

Contributor

Seems our documentation is sporadic at best about including ``​js` in our documentation.

/cc @nodejs/documentation

@trevnorris

trevnorris Jul 11, 2016

Contributor

Seems our documentation is sporadic at best about including ``​js` in our documentation.

/cc @nodejs/documentation

This comment has been minimized.

@thefourtheye

thefourtheye Jul 13, 2016

Contributor

I changed it to use ``​js` now.

@thefourtheye

thefourtheye Jul 13, 2016

Contributor

I changed it to use ``​js` now.

@jasnell

View changes

doc/api/fs.md
+```
+
+If the file is over-truncated (`len` greater than the actual size of the file),
+the file will be filled with zeroes to compensate the truncation.

This comment has been minimized.

@jasnell

jasnell Aug 8, 2016

Member

I would drop the truncation at the end of this sentence

@jasnell

jasnell Aug 8, 2016

Member

I would drop the truncation at the end of this sentence

@jasnell

View changes

test/parallel/test-fs-truncate.js
@@ -114,3 +113,43 @@ function testFtruncate(cb) {
});
});
}
+
+
+// make sure if the size of the file is smaller than the length then it is

This comment has been minimized.

@jasnell

jasnell Aug 8, 2016

Member

s/make/Make

@jasnell

jasnell Aug 8, 2016

Member

s/make/Make

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 8, 2016

Member

LGTM with a couple of nits.
ping @addaleax

Member

jasnell commented Aug 8, 2016

LGTM with a couple of nits.
ping @addaleax

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 8, 2016

Member

@thefourtheye feel free to do with my suggestion whatever you prefer, this LGTM either way!

Member

addaleax commented Aug 8, 2016

@thefourtheye feel free to do with my suggestion whatever you prefer, this LGTM either way!

thefourtheye added some commits Aug 27, 2016

test: make sure over truncation of file zero fills
If the file is over truncated, then the rest of the file should be
filled with zeroes. These tests ensure the same.
doc: improve fs.truncate functions' documentation
The default value of the `len` parameter is zero and it is included in
the documenetation.

This patch also has examples of how `ftruncate` can be used.
@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Aug 27, 2016

Contributor

@addaleax @jasnell Rebased and addressed comments. PTAL.

Contributor

thefourtheye commented Aug 27, 2016

@addaleax @jasnell Rebased and addressed comments. PTAL.

doc/api/fs.md
+For example, the following program retains only the first four bytes of the file
+
+```js
+console.log(fs.readFileSync('temp.txt', 'utf8');

This comment has been minimized.

@addaleax

addaleax Aug 27, 2016

Member

typo: missing )

@addaleax

addaleax Aug 27, 2016

Member

typo: missing )

doc/api/fs.md
+extended part is filled with null bytes ('\0'). For example,
+
+```js
+fs.readFileSync('temp.txt', 'utf-8');

This comment has been minimized.

@addaleax

addaleax Aug 27, 2016

Member

“missing” console.log?

@addaleax

addaleax Aug 27, 2016

Member

“missing” console.log?

+ assert.ifError(!err);
+ console.log(fs.readFileSync('temp.txt'));
+});
+ // prints <Buffer 4e 6f 64 65 2e 6a 73 00 00 00>

This comment has been minimized.

@addaleax

addaleax Aug 27, 2016

Member

Just a suggestion, but maybe add a second line like this?

  // prints <Buffer 4e 6f 64 65 2e 6a 73 00 00 00>
  // ('Node.js\0\0\0' in UTF8)
@addaleax

addaleax Aug 27, 2016

Member

Just a suggestion, but maybe add a second line like this?

  // prints <Buffer 4e 6f 64 65 2e 6a 73 00 00 00>
  // ('Node.js\0\0\0' in UTF8)
doc/api/fs.md
+ // prints <Buffer 4e 6f 64 65 2e 6a 73 00 00 00>
+```
+
+The last three bytes are zeroes, to compensate the over-truncation.

This comment has been minimized.

@addaleax

addaleax Aug 27, 2016

Member

nit: typically, those zeroes are referred to as null bytes… and over-truncation sounds a bit weird to me but I don’t have anything better. 😄

@addaleax

addaleax Aug 27, 2016

Member

nit: typically, those zeroes are referred to as null bytes… and over-truncation sounds a bit weird to me but I don’t have anything better. 😄

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 27, 2016

Member

Left a few comments, this still LGTM! :)

Member

addaleax commented Aug 27, 2016

Left a few comments, this still LGTM! :)

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Aug 27, 2016

Contributor

Thanks for pointing them out @addaleax. I fixed them, except the over-truncation (couldn't think of anything better :()

Contributor

thefourtheye commented Aug 27, 2016

Thanks for pointing them out @addaleax. I fixed them, except the over-truncation (couldn't think of anything better :()

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 27, 2016

Member

Thanks! As far as I am concerned this seems good to go.

Member

addaleax commented Aug 27, 2016

Thanks! As far as I am concerned this seems good to go.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 27, 2016

Member

I don’t expect any surprises, but new CI because the last one is a 404 by now: https://ci.nodejs.org/job/node-test-commit/4795/

Member

addaleax commented Aug 27, 2016

I don’t expect any surprises, but new CI because the last one is a 404 by now: https://ci.nodejs.org/job/node-test-commit/4795/

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 27, 2016

Member

LGTM!

Member

jasnell commented Aug 27, 2016

LGTM!

thefourtheye added a commit that referenced this pull request Aug 27, 2016

test: make sure over truncation of file zero fills
If the file is over truncated, then the rest of the file should be
filled with null bytes. These tests ensure the same.

PR-URL: #7648
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

thefourtheye added a commit that referenced this pull request Aug 27, 2016

doc: improve fs.truncate functions' documentation
The default value of the `len` parameter is zero and it is included in
the documenetation.

This patch also has examples of how `ftruncate` can be used.

PR-URL: #7648
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Aug 27, 2016

Contributor

Thanks for the review. Landed in c8619ea and 82c7a9c

Contributor

thefourtheye commented Aug 27, 2016

Thanks for the review. Landed in c8619ea and 82c7a9c

@thefourtheye thefourtheye deleted the thefourtheye:fs-truncate-docs branch Aug 27, 2016

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

test: make sure over truncation of file zero fills
If the file is over truncated, then the rest of the file should be
filled with null bytes. These tests ensure the same.

PR-URL: nodejs#7648
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

doc: improve fs.truncate functions' documentation
The default value of the `len` parameter is zero and it is included in
the documenetation.

This patch also has examples of how `ftruncate` can be used.

PR-URL: nodejs#7648
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

test: make sure over truncation of file zero fills
If the file is over truncated, then the rest of the file should be
filled with null bytes. These tests ensure the same.

PR-URL: #7648
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

doc: improve fs.truncate functions' documentation
The default value of the `len` parameter is zero and it is included in
the documenetation.

This patch also has examples of how `ftruncate` can be used.

PR-URL: #7648
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 30, 2016

Member

this is not landing cleanly on v4.x. Please feel free to manually backport

Member

MylesBorins commented Sep 30, 2016

this is not landing cleanly on v4.x. Please feel free to manually backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment