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

doc,fs: appendFile as an alias, and fix docs #31235

Closed
wants to merge 2 commits into from

Conversation

bengl
Copy link
Member

@bengl bengl commented Jan 7, 2020

The first commit here changes filehandle.appendFile() so that it simply calls the same backing async function as writeFile. This is because when we're dealing with file handles, it's already open, so no flags are changed. This makes the functions equivalent. This was already true, but this change makes it explicit, saving a stack frame and a few operations.

The second commit updates the docs to reflect this, and makes a few corrections to the filehandle.[read|write|append]File functions. In particular, none of them do anything with mode or flag in the options objects, so they're removed from the documentation. In addition, readFile was making reference to a path argument that isn't there.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

When operating on a FileHandle, the file has already been opened with
the flag given as an option to fs.promises.open(). This means defaulting
to 'a' has no effect here, and FileHandle#appendFile turns out to
exactly be an alias of writeFile. This is now explicit, saving a stack
frame, object copy and assignment.
* Remove `mode` and `flag` options for all three functions, since they
all ignore those options.
* Remove mention of `path` argument for `readFile`, since it isn't
actually an option to this method.
* Clarify that for file handles, `appendFile` and `writeFile` are
equivalent.
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jan 7, 2020
@bengl bengl added the doc Issues and PRs related to the documentations. label Jan 7, 2020
@Trott
Copy link
Member

Trott commented Jan 7, 2020

@nodejs/fs

@nodejs-github-bot
Copy link
Collaborator

@bengl bengl changed the title doc/fs: appendFile as an alias, and fix docs doc,fs: appendFile as an alias, and fix docs Jan 8, 2020
@Trott
Copy link
Member

Trott commented Jan 9, 2020

@nodejs/collaborators This can use some reviews.

@Trott
Copy link
Member

Trott commented Jan 9, 2020

Commit message nit for when this lands: doc: correcting filehandle.[read|write|append]File would preferably use an imperative verb:doc: correct filehandle.[read|write|append]File()?

@Trott
Copy link
Member

Trott commented Jan 10, 2020

Landed in a566c05...d4fc59d

Trott pushed a commit that referenced this pull request Jan 10, 2020
When operating on a FileHandle, the file has already been opened with
the flag given as an option to fs.promises.open(). This means defaulting
to 'a' has no effect here, and FileHandle#appendFile turns out to
exactly be an alias of writeFile. This is now explicit, saving a stack
frame, object copy and assignment.

PR-URL: #31235
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Trott pushed a commit that referenced this pull request Jan 10, 2020
* Remove `mode` and `flag` options for all three functions, since they
all ignore those options.
* Remove mention of `path` argument for `readFile`, since it isn't
actually an option to this method.
* Clarify that for file handles, `appendFile` and `writeFile` are
equivalent.

PR-URL: #31235
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
@Trott Trott closed this Jan 10, 2020
MylesBorins pushed a commit that referenced this pull request Jan 16, 2020
When operating on a FileHandle, the file has already been opened with
the flag given as an option to fs.promises.open(). This means defaulting
to 'a' has no effect here, and FileHandle#appendFile turns out to
exactly be an alias of writeFile. This is now explicit, saving a stack
frame, object copy and assignment.

PR-URL: #31235
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 16, 2020
* Remove `mode` and `flag` options for all three functions, since they
all ignore those options.
* Remove mention of `path` argument for `readFile`, since it isn't
actually an option to this method.
* Clarify that for file handles, `appendFile` and `writeFile` are
equivalent.

PR-URL: #31235
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
@codebytere codebytere mentioned this pull request Jan 16, 2020
codebytere pushed a commit that referenced this pull request Mar 14, 2020
When operating on a FileHandle, the file has already been opened with
the flag given as an option to fs.promises.open(). This means defaulting
to 'a' has no effect here, and FileHandle#appendFile turns out to
exactly be an alias of writeFile. This is now explicit, saving a stack
frame, object copy and assignment.

PR-URL: #31235
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 14, 2020
* Remove `mode` and `flag` options for all three functions, since they
all ignore those options.
* Remove mention of `path` argument for `readFile`, since it isn't
actually an option to this method.
* Clarify that for file handles, `appendFile` and `writeFile` are
equivalent.

PR-URL: #31235
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
@codebytere codebytere mentioned this pull request Mar 17, 2020
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.

4 participants