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

Added toLines. #355

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

GregorySchwartz
Copy link

Resolves #354. Probably needs a better name, should avoid partial functions, and needs verification if bang patterns are needed, especially on stream.

@Gabriella439
Copy link
Owner

@GregorySchwartz: If you don't mind, I put up an alternative fix here: #356

The main reason for doing it the other way was to ensure that toLines can handle infinite streams and operate in constant space when possible

@GregorySchwartz
Copy link
Author

For educational purposes, is the constant space due to the strict pair?

@Gabriella439
Copy link
Owner

@GregorySchwartz: The main difference is that in the streaming version the outer step' function immediately passes each complete Line to the inner step function here:

-- Emit each complete `Line`
x' <- foldM step x (NonEmpty.init lines')

... before moving on to the next Text chunk. In particular, the streaming version does not store the complete sequence of Lines inside of the accumulator as a Shell Line.

Passing completed Lines to the inner step function immediately instead of holding onto them ensures that even if there are a large (or infinite) number of output Lines then the toLines function still makes progress and doesn't wait until computing all of the Lines to begin forwarding them downstream.

@GregorySchwartz
Copy link
Author

Ah, I never thought of building a Shell outside of fold or reduce, so that makes sense.

@Gabriella439
Copy link
Owner

I'll go ahead and merge #356, then. However, I'd still like you to review #357 if you can to see if it addresses your use case before merging that one

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.

Concatenate neighboring Texts in the stream until a newline is reached
2 participants