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.

Sign up for GitHub

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

Jump to bottom

Add Deno.lines #2101

Closed
wants to merge 1 commit into from
Closed

Add Deno.lines #2101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions js/deno.ts
View file
Open in desktop
Expand Up @@ -22,6 +22,7 @@ export {
export {
copy,
toAsyncIterator,
lines,
ReadResult,
SeekMode,
Reader,
Expand Down
23 changes: 23 additions & 0 deletions js/files_test.ts
View file
Open in desktop
Expand Up @@ -29,6 +29,29 @@ testPerm({ read: true }, async function filesToAsyncIterator() {
assertEquals(totalSize, 12);
});

testPerm({ write: true, read: true }, async function fileLines() {
const encoder = new TextEncoder();
const tempDir = await Deno.makeTempDir();
const filename = tempDir + "/test.txt";

const lines = "foo\nbar\nbaz\nfizz\nbuzz";
Deno.writeFileSync(filename, encoder.encode(lines));

const f = await Deno.open(filename, "r");

const readLines = [];
for await (const line of Deno.lines(f)) {
readLines.push(line);
}

assertEquals(readLines.length, 5);
assertEquals(readLines[0], "foo");
assertEquals(readLines[1], "bar");
assertEquals(readLines[2], "baz");
assertEquals(readLines[3], "fizz");
assertEquals(readLines[4], "buzz");
});

testPerm({ write: false }, async function writePermFailure() {
const filename = "tests/hello.txt";
const writeModes: Deno.OpenMode[] = ["w", "a", "x"];
Expand Down
30 changes: 30 additions & 0 deletions js/io.ts
View file
Open in desktop
Expand Up @@ -3,6 +3,8 @@
// Documentation liberally lifted from them too.
// Thank you! We love Go!

import { TextDecoder } from "./text_encoding";

// The bytes read during an I/O call and a boolean indicating EOF.
export interface ReadResult {
nread: number;
Expand Down Expand Up @@ -159,3 +161,31 @@ export function toAsyncIterator(r: Reader): AsyncIterableIterator<Uint8Array> {
}
};
}

/** Read lines asynchronously from `r`.
*
* for await (const line of Deno.lines(reader)) {
* console.log(line);
* }
*/
export async function* lines(r: Reader): AsyncIterableIterator<string> {
const decoder = new TextDecoder();
let scratch = "";

for await (const chunk of toAsyncIterator(r)) {
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we could import code from deno_std here. The correct way to solve this is to use BufReader in std/io/bufio.ts
https://github.com/denoland/deno_std/blob/master/io/bufio.ts#L220

In order to use that code, we need to change //js/ to use extensions in the imports... (cc @kitsonk )

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that is complicated (using extensions). I was planning to tackle it in the medium term. Essentially we have to replace rollup.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should move bufio into //js then (problem is that it will likely bring all of std/io with it as dependencies)

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe "lines" should just be in deno_std instead of core..

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like BufReader provides a nice way to solve lines 👍

Reading line by line is such a common operation that IMHO it should be Deno.lines instead of living in deno_std.

It's greatly illustrated by @ry's example. This is super simple:

cat /etc/passwd |  deno eval " \
  for await (let line of Deno.lines(Deno.stdin) { \
    console.log(line.split(':')[0]) }"

This is not:

cat /etc/passwd |  deno eval " \
  import { lines } from "https://deno.land/x/io/mod.ts; \
  for await (let line of lines(Deno.stdin) { \
    console.log(line.split(':')[0]) }"

Copy link
Member

Choose a reason for hiding this comment

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

@bartlomieju I agree it would be great to have built in to the CLI... but I really want to ensure that we use the proper bufio infrastructure to solve these problems rather than having a bunch of one-off algorithms for buffering. So either we need to first be able to import deno_std code into the CLI (ideal) or move bufio from deno_std into the main repo (less ideal)

Closing this one for now. Thanks @bartlomieju !

scratch += decoder.decode(chunk);

let newLineIndex = scratch.indexOf("\n");
Copy link

Choose a reason for hiding this comment

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

Don't forget about windows carriage return line feeds \r\n. Maybe you could keep the code for finding the index of \n, but also check the index in scratch before the \n and check if it's the \r character then skip both the characters if that is the case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using something like this for the remaining lines too.

let crLf = false;
let idx = newLineIndex;
if(scratch[idx-1]=="\r") {
  idx--;
  crlf = true;
}
const line = scratch.slice(0, idx);

...

const remainingLines;
if(crLF) {
  remainingLines = scratch.split("\n");
} else {
  remainingLines = scratch.split("\r\n");
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@johnsonjo4531 you're right, that's where your implementation is better :) let's wait on BufReader it should solve this issue.


if (newLineIndex > -1) {
const line = scratch.slice(0, newLineIndex);
scratch = scratch.slice(newLineIndex + 1);
yield line;
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't only one yield per chunk mean scratch could grow linearly?
I think this newLineIndex/yield should be in a while loop.

}
}

const remainingLines = scratch.split("\n");
for (const line of remainingLines) {
yield line;
}
}