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

std/io/bufio - Parameterize input TextEncoder in readLines and readStringDelim #921

Merged
merged 3 commits into from
May 18, 2021

Conversation

carragom
Copy link
Contributor

Here is an attempt to implement what is proposed on #920, please let me know what you think.

@CLAassistant
Copy link

CLAassistant commented May 16, 2021

CLA assistant check
All committers have signed the CLA.

@carragom carragom changed the title Parameterize input TextEncoder in readLines and readStringDelim (std/io/bufio) Parameterize input TextEncoder in readLines and readStringDelim May 16, 2021
@carragom carragom changed the title (std/io/bufio) Parameterize input TextEncoder in readLines and readStringDelim std/io/bufio - Parameterize input TextEncoder in readLines and readStringDelim May 16, 2021
@kt3k
Copy link
Member

kt3k commented May 17, 2021

@carragom Thank you for your contribution! I think this is a nice addtion to the API.

Can you run deno fmt command to the changed files?

@carragom
Copy link
Contributor Author

Hi, thanks for the review, I formatted the files. But the reason the tests are failing is because the new test requires read access to be able to open the file with the encoded data. Not sure where to change that.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

@carragom Thanks! LGTM

@kt3k
Copy link
Member

kt3k commented May 18, 2021

Note: mdn doesn't seem documenting ignoreBOM option? ref: https://developer.mozilla.org/en-US/docs/Web/API/TextDecoder/TextDecoder . But it's documented in whatwg spec https://encoding.spec.whatwg.org/#interface-textdecoder , and deno actually implements it.

@kt3k kt3k merged commit b14e64e into denoland:main May 18, 2021
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.

None yet

3 participants