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

Edit Laziness #414

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

Edit Laziness #414

wants to merge 1 commit into from

Conversation

o0x2a
Copy link

@o0x2a o0x2a commented Dec 7, 2015

Function pull also cause a thunk.

Function ´pull´ also cause a thunk.
@apaleslimghost
Copy link
Collaborator

Thanks for the pull request, @Code-guru! docs/index.html shouldn't be edited directly, as it's generated from the comments in lib/index.js. Could you update your pull request to change the template please? (Note that that section of the docs has changed slightly on master)

I'm a weak +1 for the change itself. It's technically correct, but I'm not sure we should be encouraging people to use pull (higher-level transforms can often replace it). @vqvu, what do you think?

@vqvu
Copy link
Collaborator

vqvu commented Dec 7, 2015

I'm also a weak +1, but for the reason that this sentence is not meant to be an exhaustive list of all methods that consume a stream---that's what the Consumption section is for.

pull's use is generally discouraged, but it is useful in for implementing higher-level transforms when the ones we already have are not adequate (e.g., @svozza's sortedMergeBy). It's use is not discouraged to the same level as pause and resume—I've never seen an external usecase for pause, and resume is only useful for changing the backpressure behavior, like in throttle. But since we already mention resume in that sentence, adding pull probably doesn't hurt our messaging any.

@apaleslimghost
Copy link
Collaborator

It's not clear that the Consumption section is meant to be that exhaustive list, given that only some methods mention they cause a thunk (implying that the rest don't). Maybe we should add a short intro to that section, reiterating parts of Laziness.

@svozza
Copy link
Collaborator

svozza commented Dec 7, 2015

But since we already mention resume in that sentence, adding pull probably doesn't hurt our messaging any.

I guess the other way to go could be to remove resume from the sentence.

@vqvu
Copy link
Collaborator

vqvu commented Dec 7, 2015

It's not clear that the Consumption section is meant to be that exhaustive list, given that only some methods mention they cause a thunk (implying that the rest don't).

Well, the terminology that we use now is <em>consume</em> the stream, so a section named Consumption seems self-explanatory. pull says it "consumes a single item from the Stream" and pipe says it "pull[s] all the data from the source Highland Stream and write it to the destination". All of the methods mentions consuming the stream. So, I don't think it's confusing any longer.

I guess the other way to go could be to remove resume from the sentence.

I'd be OK with that. I'd also be OK with adding pull and taking out resume. pull is useful in certain circumstances, and we already have a warning in its entry saying "you probably don't need to use this".

@vqvu
Copy link
Collaborator

vqvu commented Dec 16, 2015

@Code-guru, if you're still interested, can you update this PR to change the docs/templates/base.html file? You can also remove the part that mentions resume. Then we can merge this.

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.

4 participants