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: document intention and caveats of Buffer API #6020

Closed
wants to merge 2 commits into from
Closed

fs: document intention and caveats of Buffer API #6020

wants to merge 2 commits into from

Conversation

seishun
Copy link
Contributor

@seishun seishun commented Apr 2, 2016

Pull Request check-list

Please make sure to review and check all of these items:

  • Is the commit message formatted according to [CONTRIBUTING.md][0]?

Affected core subsystem(s)

fs

Description of change

This should prevent potential confusion of newcomers.

@mscdex mscdex added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Apr 2, 2016
`fs` functions support passing and receiving paths as both strings and Buffers.
The latter is intended for filesystems which support non-UTF-8 filenames (except
NTFS, where the conversion to UTF-8 is done automatically) and is normally not
needed.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps,

`fs` functions support passing and receiving paths as both strings
and Buffers. The latter is intended to make it easier to work with
filesystems that allow for non-UTF-8 filenames. For most typical
uses, working with paths as Buffers will be unnecessary.

*Note* that on certain file systems (such as NTFS and HFS+) Node.js
will always encode filenames as UTF-8. On such file systems, passing 
non-UTF-8 encoded Buffers to `fs` functions will not work as expected.

@jasnell
Copy link
Member

jasnell commented Apr 2, 2016

Good idea generally LGTM but offered some rewording.

@seishun
Copy link
Contributor Author

seishun commented Apr 3, 2016

The latter is intended to make it easier to work with filesystems that allow for non-UTF-8 filenames.

Actually it's impossible to work with non-UTF-8 filenames using the string API, so this is a bit misleading.

on certain file systems (such as NTFS and HFS+) Node.js will always encode filenames as UTF-8.

Somewhat misleading too. This is only true on NTFS. On NFS+, Node.js doesn't touch filenames - UTF-8 is enforced by the file system.

@jasnell
Copy link
Member

jasnell commented Apr 4, 2016

`fs` functions support passing and receiving paths as both strings
and Buffers. The latter is intended to make it possible to work with
filesystems that allow for non-UTF-8 filenames. For most typical
uses, working with paths as Buffers will be unnecessary.

*Note* that on certain file systems (such as NTFS and HFS+) filenames
will always be encoded as UTF-8. On such file systems, passing 
non-UTF-8 encoded Buffers to `fs` functions will not work as expected.

@addaleax
Copy link
Member

addaleax commented Apr 4, 2016

As this explicitly targets newcomers – it might be a good idea to mention that NTFS and HFS+ are the Windows and OS X standard file systems, respectively, if that’s not too distracting from the point?

@seishun
Copy link
Contributor Author

seishun commented Apr 12, 2016

@jasnell Updated using your suggestions, plus an addition from me. I thought we should make it clear somewhere that the string API is just Buffer API with automatic conversion to and from UTF-8.

@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

LGTM

jasnell pushed a commit that referenced this pull request Apr 18, 2016
PR-URL: #6020
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

Landed in cf29b2f

@jasnell jasnell closed this Apr 18, 2016
MylesBorins pushed a commit that referenced this pull request Apr 19, 2016
PR-URL: #6020
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
PR-URL: #6020
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
PR-URL: #6020
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 20, 2016
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
PR-URL: #6020
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

@jasnell is this relevant to lts?

@jasnell
Copy link
Member

jasnell commented Apr 21, 2016

No. This is specific to the new buffer support in the fs module. Not
applicable to lts
On Apr 20, 2016 11:49 PM, "Myles Borins" notifications@github.com wrote:

@jasnell https://github.com/jasnell is this relevant to lts?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6020 (comment)

MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
PR-URL: #6020
Reviewed-By: James M Snell <jasnell@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
PR-URL: nodejs#6020
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
PR-URL: #6020
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants