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

Added read file str #276

Merged
merged 7 commits into from Apr 13, 2019

Conversation

4 participants
@zekth
Copy link
Contributor

commented Mar 14, 2019

As discussed in : denoland/deno#1925
Moving readFileStr / readFileStrSync to fs-extra

@j-f1

This comment has been minimized.

Copy link

commented Mar 14, 2019

How about defaulting encoding to utf-8?
Edit: that’s actually the default for TextDecoder already.

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

How about defaulting encoding to utf-8?

Definitely legit.

* Read file synchronously and output it as a string.
*
* @param filename File to read
* @param encoding Encoding of the file. Default = "utf-8"

This comment has been minimized.

Copy link
@j-f1

j-f1 Mar 14, 2019

JSDoc supports specifying defaults:

Suggested change
* @param encoding Encoding of the file. Default = "utf-8"
* @param [encoding="utf-8"] Encoding of the file.

However, since TextDecoder defaults to ITF-8, should this just pass on undefined?

This comment has been minimized.

Copy link
@zekth

zekth Mar 14, 2019

Author Contributor

JSDoc supports defaults Without the type definition? Thought it was needed due to my linter. My bad.
About the undefined value, it may be better to have the default in the jsdoc instead of using implicit default for automatic API documentation. Don't you think?

* @param filename File to read
* @param [encoding="utf-8"] Encoding of the file
*/
export async function readFileStr(

This comment has been minimized.

Copy link
@axetroy

axetroy Mar 14, 2019

Contributor

how about

Suggested change
export async function readFileStr(
export async function readFile(

This comment has been minimized.

Copy link
@zekth

zekth Mar 14, 2019

Author Contributor

May be confusing with the Deno.readFile no?

This comment has been minimized.

Copy link
@axetroy

axetroy Mar 14, 2019

Contributor

@zekth
fs module is used to add some quick and easy methods

Returning a string is the most common method for developer

In most cases, Deno.readFile returns the Reader is not commonly used

So I don’t think these conflicts.

This comment has been minimized.

Copy link
@zekth

zekth Mar 14, 2019

Author Contributor

Totally agree with you. First it was named like this to be in Deno and not in fs-extra as you can see in the ref.

*/
export async function readFileStr(
filename: string,
encoding: string = "utf-8"

This comment has been minimized.

Copy link
@axetroy

axetroy Mar 14, 2019

Contributor

encoding should be an options object in case of other fields in the future. eg flag

Suggested change
encoding: string = "utf-8"
options: ReadOptions = {}

This comment has been minimized.

Copy link
@zekth

zekth Mar 14, 2019

Author Contributor

Legit. Will add.

@axetroy axetroy referenced this pull request Mar 14, 2019

Closed

implement fs-extra api for fs modules #261

12 of 13 tasks complete
Show resolved Hide resolved fs/read_file.ts Outdated

@zekth zekth referenced this pull request Mar 18, 2019

Merged

Added EOL detect / format #289

@zekth zekth force-pushed the zekth:readfilestr branch from b1cf10b to ae08bd3 Mar 19, 2019

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

Rebased and done the reviews @ry

@ry

ry approved these changes Apr 13, 2019

Copy link
Contributor

left a comment

LGTM

@ry ry merged commit b462ad2 into denoland:master Apr 13, 2019

2 checks passed

denoland.deno_std #20190319.13 succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@axetroy

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

Since there is a read interface, should there be a write interface?

@zekth zekth deleted the zekth:readfilestr branch Apr 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.