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

Remove parseFile from toml #361

Merged
merged 1 commit into from
Apr 25, 2019
Merged

Conversation

jcao219
Copy link
Contributor

@jcao219 jcao219 commented Apr 25, 2019

Feel free to close if this is not the right direction

@CLAassistant
Copy link

CLAassistant commented Apr 25, 2019

CLA assistant check
All committers have signed the CLA.

toml/parser.ts Outdated
}
const strFile = await readFileStr(filePath);
return parse(strFile);
}
Copy link
Member

Choose a reason for hiding this comment

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

Actually I'd rather that we remove the parseFileSync and leave it up to the caller to load the data. The TOML module should only deal with parsing, not with I/O.

Would you mind a revised PR that removes parseFileSync (or just moves it to the test module - if that is needed there)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes a lot of sense to me!

} else {
inLitteral = true;
inLiteral = true;
Copy link
Member

Choose a reason for hiding this comment

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

Ah - thanks!

@jcao219 jcao219 changed the title Make toml parseFile async Remove parseFile from toml Apr 25, 2019
@jcao219 jcao219 force-pushed the toml-async-parsefile branch 2 times, most recently from 6d21401 to 883f09c Compare April 25, 2019 22:02
Copy link
Contributor

@zekth zekth left a comment

Choose a reason for hiding this comment

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

As parseFile has been deleted, please update the README.md

@jcao219
Copy link
Contributor Author

jcao219 commented Apr 25, 2019 via email

Copy link
Contributor

@zekth zekth left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit 0431b2f into denoland:master Apr 25, 2019
@jcao219 jcao219 deleted the toml-async-parsefile branch April 26, 2019 01:44
ry pushed a commit to ry/deno that referenced this pull request Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants