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

Add xeval #581

Merged
merged 4 commits into from Sep 11, 2019

Conversation

@nayeemrmn
Copy link
Contributor

commented Sep 4, 2019

Experimenting with moving deno xeval here.
CC @bartlomieju @kevinkassimo

@nayeemrmn nayeemrmn force-pushed the nayeemrmn:xeval branch 3 times, most recently from 1e152cd to 185159d Sep 4, 2019
@kevinkassimo

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

It is embarrassing, but I have to admit I just spot a bug with the chunks implementation I did before... The multi-char delimiter matching is throwing away incorrect parts, resulting in things like !DEDELIM!DELIM! incorrectly delimited with -d 'DELIM' set... We need KMP or Boyer Moore for this

@nayeemrmn nayeemrmn force-pushed the nayeemrmn:xeval branch from 185159d to 1d6dbb6 Sep 4, 2019
@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

@kevinkassimo maybe it's worth fixing the problem in this PR? IMHO it makes sense to move xeval to deno_std

@nayeemrmn nayeemrmn force-pushed the nayeemrmn:xeval branch 5 times, most recently from 2db97bc to ef76abf Sep 4, 2019
@kevinkassimo

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

@bartlomieju I had a PR fixing the behavior in the deno repo. I’ll wait for it to be reviewed and see we can completely move it here

@kevinkassimo

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

I was also thinking if we should move chunks to io/? Since it takes a Reader interface and might be also useful in other scenarios.

kevinkassimo and others added 3 commits Sep 4, 2019
Co-authored-by: Nayeem Rahman <muhammed.9939@gmail.com>
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:xeval branch from f40974f to 94a7c85 Sep 5, 2019
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:xeval branch from 94a7c85 to 05568c1 Sep 5, 2019
@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

I was also thinking if we should move chunks to io/? Since it takes a Reader interface and might be also useful in other scenarios.

Yes that makes sense, I'd also suggest adding lines function that is an alias for chunks - it's been requested many times.

@nayeemrmn

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

Well it behaves like String::split() but for Readers. I would call it readAndSplit() or maybe even just split(). chunks() is vague.

@kevinkassimo

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

@Narendra-Kamath I like the idea of including split in the name, but readAndSplit sounds a bit too long? Maybe readSplit() or readDelim() (align with readLine)? Also we can add a maxSplit property to it, such that readSplit(reader, "\n", { maxSplit: 2 }) will yield at most 2 strings.

Also we might want to distinguish async function* readSplit(r: Reader, delim: Uint8Array): AsyncIterableIterator<Uint8Array> and readStrSplit(r: Reader, delim: string): AsyncIterableIterator<string>. (This change should be quite simple)

Also I noticed https://github.com/denoland/deno_std/blob/master/io/readers.ts . We can maybe also implement a DelimitedReader

@nayeemrmn

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

distinguish async function* readSplit(r: Reader, delim: Uint8Array): AsyncIterableIterator<Uint8Array> and readStrSplit(r: Reader, delim: string): AsyncIterableIterator<string>.

Yeah. Most accurately it reads, decodes to string and splits. readStrSplit() checks every box.

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

Also I noticed https://github.com/denoland/deno_std/blob/master/io/readers.ts . We can maybe also implement a DelimitedReader

👍 additionally Rust has lines method implemented on BufReader - I believe we should replicate this function on Deno's BufReader

As for the renaming of chunks I'd suggest readDelimitedChunks.

@nayeemrmn can you update the PR? There are a few issues waiting on this functionality :)

@nayeemrmn

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

@bartlomieju I think moving chunks() should be a separate PR. It would get merged a lot sooner and I can rebase this one. @kevinkassimo?

@kevinkassimo

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

I'm fine with landing this first (as long as we don't expose chunks or equivalent here)

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

@ry please review

@ry
ry approved these changes Sep 11, 2019
Copy link
Contributor

left a comment

LGTM

@ry ry merged commit a2cae36 into denoland:master Sep 11, 2019
5 checks passed
5 checks passed
denoland.deno_std Build #20190905.3 succeeded
Details
denoland.deno_std (Linux) Linux succeeded
Details
denoland.deno_std (Mac) Mac succeeded
Details
denoland.deno_std (Windows) Windows succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@nayeemrmn nayeemrmn deleted the nayeemrmn:xeval branch Sep 11, 2019
ry added a commit to ry/deno that referenced this pull request Oct 9, 2019
Co-authored-by: Nayeem Rahman <muhammed.9939@gmail.com>


Original: denoland/deno_std@a2cae36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.