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.readFile() can accept a string for the second argument, resolves #1797 #1806

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented May 27, 2015

No description provided.

@mscdex mscdex added the doc Issues and PRs related to the documentations. label May 27, 2015
@Fishrock123
Copy link
Contributor

@Trott could you also add it to readFileSync? Thanks!

@Trott
Copy link
Member Author

Trott commented May 27, 2015

Sure! Looking at the code in fs.js, it looks like I should also add a note for writeFile(), appendFile(), and their synchronous counterparts as well.

@Trott
Copy link
Member Author

Trott commented May 27, 2015

Looking at the other *Sync() function docs, I'm wondering if it's superfluous to add it there. It seems like the convention is to note that it is a synchronous version of the async function, and only note any differences.

readFileSync() doc is a little odd that way, but you can make the case that it's worth repeating the stuff about returning buffer vs. string because now we're talking about a function's return value and not a value passed to a callback.

@Trott
Copy link
Member Author

Trott commented May 27, 2015

I've pushed a new commit that updates the writeFile() and appendFile(). Holding off on readFileSync()/ writeFileSync()/appendFileSync() for feedback on whether it's needed.

Fishrock123 pushed a commit that referenced this pull request May 27, 2015
Fixes: #1797
PR-URL: #1806
Reviewed-By: fishrock123@rocketmail.com
@Fishrock123
Copy link
Contributor

Thanks, landed in ff79449 :)

@rvagg rvagg mentioned this pull request May 30, 2015
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
Fixes: nodejs/node#1797
PR-URL: nodejs/node#1806
Reviewed-By: fishrock123@rocketmail.com
@Trott Trott deleted the encoding branch October 14, 2021 13:47
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants