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

Fixup documentation. #397

Merged
merged 9 commits into from
Nov 26, 2015
Merged

Fixup documentation. #397

merged 9 commits into from
Nov 26, 2015

Conversation

vqvu
Copy link
Collaborator

@vqvu vqvu commented Nov 19, 2015

I only meant to update uniq to stop saying that it's implemented via uniqBy but ended up doing a pass over the entire documentation...

Most of the changes are adding formatting and type annotations for arguments, though I've clarified the wording in certain cases.

I also have a question. The docs mentions this at one point

To get the map iterator to be called, we must pull some data from the Stream. This is called a _thunk_, and some Highland methods will cause them (eg, each, done, apply, toArray, pipe, resume).

My familiarity with the term thunk is in this context: https://en.wikipedia.org/wiki/Thunk. Basically, it's a chunk of code, which is not the way it's being used here. Is this usage of the term common?

@apaleslimghost
Copy link
Collaborator

Actual thunks in the Haskell sense are a fairly common way of implementing lazy streams, as:

data Thunk a = Thunk (() -> a)
data Stream a = Nil | Cons a (Thunk (Stream a))

To actually get data from the stream you force the thunk:

force :: Thunk a -> a
force t = t ()

toList :: Stream a -> Thunk [a]
toList Nil = Thunk \_ -> []
toList (Cons head tail) = Thunk \_ -> ([head] :: force (toList (force tail)))

Since all the forcing in toList happens inside a thunk, nothing is evaluated until you actually force the thunk it returns.

I think "causing a thunk" actually refers to forcing. I've no idea how Highland's laziness is actually implemented though 😁


Code examples are written in an eager pseudo-Haskell. Here's a more complete implementation of Javascript (well, Sweet.js) lazy streams.

@@ -2241,8 +2249,12 @@ Stream.prototype.uniqBy = function (compare) {
exposeMethod('uniqBy');

/**
* Takes all unique values in a stream.
* It uses uniqBy internally, using the strict equality === operator to define unicity
* Filters out all duplicate values from the stream and keep only the first
Copy link
Collaborator

Choose a reason for hiding this comment

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

/s/keep/keeps

@svozza
Copy link
Collaborator

svozza commented Nov 19, 2015

Nice work!

@vqvu
Copy link
Collaborator Author

vqvu commented Nov 19, 2015

I see. That makes sense. resume in Highland is equivalent to force here, and all of those transforms call resume. I think I was confused because of the use of "cause".

3.0.0 streams are implemented in a similar way as your Haskell example. Every stream has a generator function that gets called when you call resume and is responsible for generating its data. There's obviously no Cons, but that's because the generator is expected to be stateful and not pure. I guess the generator is the thunk here.

I think I'll try to reword this so that it doesn't suggest that pulling data from the stream is a thunk.

@vqvu
Copy link
Collaborator Author

vqvu commented Nov 19, 2015

Ok. I've addressed @svozza's comment and replaced "cause a thunk" with the slightly more verbose "consume the stream". Better terminology welcome.

@apaleslimghost
Copy link
Collaborator

+1 for "consume the stream". I wonder what @caolan thinks about it?

* var checkExists = _.wrapCallback(fs.exists);
* function checkExists(file) {
* return _(function (push, next) {
* fs.exists(file, function (exists) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be using deprecated Node.js functions in the examples?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought about just changing it to fs.access, but the other side of that argument is, "should we be using a function that doesn't exist in 0.10 when we technically support it?"

That said, Node 4 is the LTS version, so maybe not many people use 0.10 anymore. I don't feel strongly about it though, so if you don't think it matters, then I'm happy to change it.

@vqvu
Copy link
Collaborator Author

vqvu commented Nov 26, 2015

I added some deprecation warnings for the things we are changing in 3.0.0, and changed the use of fs.exists to fs.access (per @svozza's suggestion).

@svozza
Copy link
Collaborator

svozza commented Nov 26, 2015

Good idea with the deprecation warnings. LGTM.

vqvu added a commit that referenced this pull request Nov 26, 2015
@vqvu vqvu merged commit dc08721 into caolan:master Nov 26, 2015
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