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

refactor: export chunks in std/io #3497

Closed
wants to merge 5 commits into from

Conversation

bartlomieju
Copy link
Member

This PR addresses old TODO to export chunks utility method in std.

  • move chunks from std/xeval/mod.ts to std/io/bufio.ts
  • add lines helper which is chunks(reader, "\n")
  • add simple test case

CC @kevinkassimo @David-Else

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Dec 14, 2019

We talked about naming this in denoland/std#581. I like readDelim().

@bartlomieju
Copy link
Member Author

@nayeemrmn thanks for pointing this out, I forgot about it. As for the naming I'm open to changes.

Reading my comment from that issue I had idea to expose chunks and lines on BufReader, but since BufReader is used to operate strictly on Uint8Array and chunks/lines convert to string I believe making them a separate functions is fine.

Furthermore, thinking about it now, it might make more sense to place those functions in strings module instead.

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Dec 14, 2019

@bartlomieju Ah, right. I think we wanted to remove the tacked-on encoding/decoding from this and make it binary. Then call it readDelim() and implement a readStringDelim() on top of it. I don't know how that fits into BufReader... that's a stateful thing and maybe these generators should just stay separate from it.

Edit: BufReader has readSlice() and readString(). These would be analogous to those but as self-contained generators.

@bartlomieju
Copy link
Member Author

@bartlomieju Ah, right. I think we wanted to remove the tacked-on encoding/decoding from this and make it binary. Then call it readDelim() and implement a readStringDelim() on top of it. I don't know how that fits into BufReader... that's a stateful thing and maybe these generators should just stay separate from it.

Edit: BufReader has readSlice() and readString(). These would be analogous to those but as self-contained generators.

Then maybe BufReader.readStringChunks and BufReader.readStringLines could work

std/io/bufio.ts Outdated
Comment on lines 599 to 601
for await (const line of chunks(reader, "\n")) {
yield line;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

yield* chunks(reader, "\n");

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Dec 14, 2019

@bartlomieju How would you go about adding them on BufReader anyway? They would be generator methods. Those would return iterables. Advancing these iterables would affect the state of the BufReader, but that's not how generator methods should work.

Just add a top-level readDelim() (binary), readStringDelim() (strings) and readLines() (strings, newline delim) where each shells out to the last. Swap out *Delim for whatever suffix.

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Dec 15, 2019

Not really related, but maybe a Boyer-Moore-Horspool implementation could work better than KMP (I realized that v8 uses this for indexOf), like this one: https://github.com/mscdex/streamsearch/blob/master/lib/sbmh.js

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.

3 participants