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

BREAKING: Call _end via read method #43

Closed
wants to merge 10 commits into from

Conversation

jeswr
Copy link
Collaborator

@jeswr jeswr commented Mar 13, 2022

resolves #25 #35

@coveralls
Copy link

coveralls commented Mar 13, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling ed8d491 on jeswr:breaking/end-on-read into 7a089e3 on RubenVerborgh:main.

@jeswr
Copy link
Collaborator Author

jeswr commented Mar 13, 2022

@rubensworks - in your implementation of toArray you removed the listeners rather than destroying the iterator when the limit was reached - is there a particular reason for this?

You will see that I have changed this behavior in ed8d491 - this is because as discussed here removing a 'data' listener should not pause the stream, so in the case of your tests with infinite iterators the test suite was entering an infinite loop. An alternative is to call .pause inside toArray; but I would hazard against this as it would give unexpected behavior if other data listeners are attached.

@RubenVerborgh
Copy link
Owner

@jeswr Thanks for your work.

There are quite some unrelated changes in this PR, which makes it hard for me to assess. Could we do this one step by step, possibly with different PRs for things like linting etc.? Changes like this one are a bit distracting; I'd want to focus on one thing at a time.

@rubensworks
Copy link
Collaborator

@rubensworks - in your implementation of toArray you removed the listeners rather than destroying the iterator when the limit was reached - is there a particular reason for this?

The reasoning here was that the iterator could still be used after calling toArray(). For example for splicing of multiple arrays from the iterator, or perhaps even resuming the stream after first consuming an array.
But given the discussion in comunica/comunica#950, it might make sense to change this though.

@RubenVerborgh
Copy link
Owner

In general, I like to leave options open. So the idea is that, after toArray is finished. the derived iterator has stopped existing and the source iterator is free again. This default enables cases such as the ones @rubensworks describes.

But destroySource: true might be appropriate.

@jeswr jeswr mentioned this pull request Apr 28, 2022
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.

Consider removing autoStart, or at least disabling it by default
4 participants