doc: updated readFileSync in fs.md#12800
doc: updated readFileSync in fs.md#12800thelostone-mc wants to merge 2 commits intonodejs:masterfrom
Conversation
|
prefix the PR and commit with the subsystem you changed. In this case doc |
doc/api/fs.md
Outdated
There was a problem hiding this comment.
macOS, Linux, and Windows
We use the Oxford comma
There was a problem hiding this comment.
move "see example below" to the last line of the paragraph
doc/api/fs.md
Outdated
There was a problem hiding this comment.
- remove "In contrast"
- add
the- "on FreeBSD, the content"
doc/api/fs.md
Outdated
There was a problem hiding this comment.
You should explain which file? as it's a directory?
There was a problem hiding this comment.
@refack Would that mean :
On FreeBSD, the contents of the files in the directory will be returned.
There was a problem hiding this comment.
If that is what happens, then yes, exactly.
BTW: that's strange, how do you know when one file ends and another begins... 🤔
There was a problem hiding this comment.
I'm tempted to set up FreeBSD and check it out now 😛
|
Thank you! |
|
Thank you very much for the contribution 🥇 |
|
@refack Haha!! Well next time you won't be able to point things like this out in my next PR 😎 |
I agree, the best way to learn is by doing! |
|
@fhalde Thanks !! 🕺 |
doc/api/fs.md
Outdated
There was a problem hiding this comment.
What does this mean exactly? Is it just all of the files concatenated together one after the other? With or without boundaries?
There was a problem hiding this comment.
I've been working (slowly) on getting as much consistency as possible in the Note sections in the docs... Can you please make this...
*Note*: The behavior...
... and do not bold the entire paragraph.
doc/api/fs.md
Outdated
There was a problem hiding this comment.
Synchronous methods don't accept callbacks.
doc/api/fs.md
Outdated
There was a problem hiding this comment.
I think we should be more general about what functions are affected since it would also include the async counterpart as well as anyone using fs.open('/path/to/dir') with fs.read() or their sync counterparts for example.
There was a problem hiding this comment.
Won't the example clear this up? I agree your comment give room for less confusion
But the line below would clear up that confusion:
When the given path identifies a directory, the behavior of fs.readFile() and fs.readFileSync() is platform specific
There was a problem hiding this comment.
Except those methods both implicitly use fs.open()/fs.read(). So really the note should be above one of those instead, but many people use readFile() as well. That is why I suggested possibly putting the note for both readFile() and fs.read() (and their sync counterparts).
doc/api/fs.md
Outdated
There was a problem hiding this comment.
I've been working (slowly) on getting as much consistency as possible in the Note sections in the docs... Can you please make this...
*Note*: The behavior...
... and do not bold the entire paragraph.
doc/api/fs.md
Outdated
There was a problem hiding this comment.
I would make this first line...
When the given path identifies a directory, the behavior of `fs.readFile()` and
`fs.readFileSync()` is platform specific. On macOS, Linux and Windows, an
error will be returned. On FreeBSD...
There was a problem hiding this comment.
@jasnell If i were to do this, this would be first line fs.readFile().
Would we need the same to be copied even for fs.readFileSync() ?
Also , would I be giving examples for both sync and async counterparts ?
e358d43 to
338bc1b
Compare
doc/api/fs.md
Outdated
There was a problem hiding this comment.
Nit: platform specific -> platform-specific
There was a problem hiding this comment.
(Although I know you didn't write that text. But while we're here...)
There was a problem hiding this comment.
Consider it done.
doc/api/fs.md
Outdated
There was a problem hiding this comment.
Nit: platform specific -> platform-specific
There was a problem hiding this comment.
Please put a space after the period.
There was a problem hiding this comment.
@Trott comma after Linux instead of and. Are you sure about that?
There was a problem hiding this comment.
@adityaanandmc Comma after Linux and keep the and:
On macOs, Linux, and Windows, an error will be returned.
That would be a serial comma and be in accordance with the style guide for Node.js docs.
doc/api/fs.md
Outdated
There was a problem hiding this comment.
Is this supposed to say in the directory rather than in directory?
doc/api/fs.md
Outdated
There was a problem hiding this comment.
Would it be too imprecise to say When the path is a directory instead of When the given path identifies a directory? The former seems clearer to me, even if the latter may be more precise.
There was a problem hiding this comment.
Makes sense. Will change it
Trott
left a comment
There was a problem hiding this comment.
I left a bunch of small comments, although most of them probably shouldn't stop this from landing if the contributor is exhausted from requests for small changes. :-D (Also, whoever lands it can probably fix any of them that are important.)
|
I think a similar note should be made above |
|
@Trott Looked into the comments and changes everything accordingly except the
Well, any number of small fixes -> still worth it for documentation, so it's alright! |
4933ce0 to
b4e42e0
Compare
doc/api/fs.md
Outdated
There was a problem hiding this comment.
@Trott is not just being picky. It's called the "oxford comma" and we ask for its use in our style guide
There was a problem hiding this comment.
Should be... It doesn't work in the GitHub web view but will work in https://nodejs.org/api/fs.html#fs_fs_readfilesync_file_options
doc/api/fs.md
Outdated
doc/api/fs.md
Outdated
There was a problem hiding this comment.
$ mkdir test && echo hello > test/1 && echo world > test/2
$ node -p "fs.readFileSync('test')"
<Buffer 03 00 00 00>
$ node -p "fs.readFileSync('/tmp')"
<Buffer 03 00 00 00 00 00 00>
Doesn't look like the contents 😄
FreeBSD 11
There was a problem hiding this comment.
This buffer doesn't look like a readable utf8 string too
There was a problem hiding this comment.
Could you try it with : node -p "fs.readFileSync('test', 'utf-8')"
I'm on MacOS, so I tried it with fs.readFileSync() on a file instead of a directory and it worked as expected!
There was a problem hiding this comment.
@adityaanandmc No problem
$ node -p "fs.readFileSync('test', 'utf-8')"
�
The same as
Buffer.from([3, 0, 0, 0]).toString("utf-8")What did you expect ? 😃
Yes, macOS works as expected(throws an error), but the subject is FreeBSD, and "test" is a directory.
On FreeBSD, the contents of the files in the directory will be returned.
This is wrong.
Linux(Darwin too) doesn't allow using the read syscall to read bytes from a directory, but FreeBSD does 😐
And the result doesn't look like the contents of the files in the directory
There was a problem hiding this comment.
@s0m3on3 so what are those?
$ node -p "fs.readFileSync('test')"
<Buffer 03 00 00 00>
$ node -p "fs.readFileSync('/tmp')"
<Buffer 03 00 00 00 00 00 00>
There was a problem hiding this comment.
@refack I don't know. The directory contents as a file, some internal structure
There was a problem hiding this comment.
I suggest:
On FreeBSD, a representation of the directory's contents will be returned.
|
I spun up a freeBSD vm and got this: > fs.readFileSync('test', {encoding:'utf-8'}).replace('\u0000', ' ')
'�M\u0005 \f\u0000\u0004\u0001.\u0000\u0000\u0000\u0006�\u0004\u0000\f\u0000\u0004\u0002..\u0000\u0000�M\u0005\u0000\u001c\u0000\u0004\u0013this_is_a_directory\u0000D�\u0004\u0000\u0010\u0000\b\u0005hello\u0000��N�\u0004\u0000�\u0001\b\bduck.txt\u0000\u0001v�\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000'
> fs.readFileSync('test', {encoding:'utf-8'}).replace(/\u0000/g, ' ')
'�M\u0005 \f \u0004\u0001. \u0006�\u0004 \f \u0004\u0002.. �M\u0005 \u001c \u0004\u0013this_is_a_directory D�\u0004 \u0010 \b\u0005hello ��N�\u0004 �\u0001\b\bduck.txt \u0001v� '
> fs.readdirSync('test')
[ 'duck.txt', 'hello', 'this_is_a_directory' ]
>So IMHO we should say: |
doc/api/fs.md
Outdated
There was a problem hiding this comment.
I suggest:
On FreeBSD, a representation of the directory's contents will be returned.
Updated fs.md stating fs.readFileAsync is platform specific Refs: nodejs#10962
|
@refack Done!! |
|
Landed in: 2614d24 |
|
Post land CI (Just lint): https://ci.nodejs.org/job/node-test-linter/8838/ |
* Updated fs.md stating fs.readFileAsync is platform specific * Fix formatting of `note`s PR-URL: nodejs#12800 Refs: nodejs#10962 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Refs: #10962
Updated fs.md stating fs.readFileAsync is platform specific
Documents Updated:
Checklist
Affected core subsystem(s)
doc