-
Notifications
You must be signed in to change notification settings - Fork 14
Stream functions #57
Comments
@anovstrup this looks great so far, let us know when it's ready for review. One note - we just rebooted the history of base so for your next PR be sure to delete your local @runarorama we should make sure to do a squash merge for this PR to avoid reintroducing the excessively huge history. |
How's this going? I'm pretty interested in getting these functions! |
@runarorama I just pushed the latest, which adds some tests, corrects the implementation of Status:
Since things have gotten busy at my day job and my night/weekend job (husband/dad), I was thinking about recruiting collaborators on the Slack hackathon channel to help push this over the finish line. |
@runarorama Alternatively to my recruiting helpers on this PR, maybe you'd prefer to review this one as it stands and then add the missing stuff over time in separate PRs? |
I'm already using your code in some definitions, so I'm happy to review and merge what's here now. |
+1 for getting these functions into base |
What was the rationale for the decision to err on the side of emitting eagerly? It seems more ergonomic to me to apply transformations that both take and return a delayed stream, and then just force it once at the end. For example, compare the following with a map that emits eagerly:
Whereas with a map that preserves the delayedness, the line in main can instead be
|
@sullyj3 To the best of my recollection, I thought of it as a fairly arbitrary choice but one I wanted to be generally consistent about. Performance considerations tilted the scales in favor of eager effects — there's an overhead to suspending, and I figured users could readily suspend if they wanted to. And, of course, it's easy to implement lazy versions of the eager functions in terms of the eager ones. |
Makes sense! |
DRAFT: docs and tests in progress
This PR adds a bunch of Stream functions (and a handful of Either functions that are used in the Stream function implementations). Design constraints include:
Stream.Nat.all
) and in others because it seemed like a delayed result would better suit the most likely use cases (e.g.,Stream.cons
)Interesting/Controversial Decisions
I had a hard time deciding whether functions that ignore the return value of a streaming computation (e.g.,
Stream.toList
) should be parameterized by the return type of that computation (i.e., shouldStream.toList : '{g, Stream a} () ->{g} [a]
or: '{g, Stream a} r ->{g} [a]
?). On the one hand, using()
makes it explicit that the function does not distinguish among return values of the stream, but on the other hand it's less flexible for users who are forced to passvoid s
rather than justs
for a non-unit-valued streams
. Ultimately, I decided to favor flexibility.Issues
The pull request shows a name conflict for
Stream.toList
, although the patch includes a term replacement entry.Code review
The changes summarized below are available for you to review, using the following command:
New name conflicts:
Updates:
Added definitions:
Name changes:
The text was updated successfully, but these errors were encountered: