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

draft: synchronousTransformIterator #77

Closed
wants to merge 1 commit into from

Conversation

jeswr
Copy link
Collaborator

@jeswr jeswr commented Jul 4, 2022

As discussed in #75 (comment).

TODOs

  • Add more coverage
  • See if
    if (!this.root.readable) {
    this.readable = false;
    // TODO: See if this should be here
    if (this.root.done)
    this.close();
    return null;
    }
    is necessary. @RubenVerborgh the question here is - can we just assume the root is .readable if the parent source is .readable. If we assume that all of the intermediary sources are SynchronousTransformIterators as is the case when doing chaining then this will be true; but I am not sure if we should be making this assumption.

Quick testing indicates that this results in a ~2x performance increase on the tests in #75

@jacoscaz
Copy link
Collaborator

jacoscaz commented Jul 4, 2022

Should this be a part of the changes summarized at #44 (comment) ?

@jacoscaz
Copy link
Collaborator

@jeswr is this something you're still working on? Would this be releasable as 3.x or 4.x?

@jeswr
Copy link
Collaborator Author

jeswr commented Aug 26, 2022

@jacoscaz The changes are still valid - and there are no breaking changes. I can try and work on the test coverage over the weekend depending on my time (though I'm looking a bit stretched thin at this point).

@jacoscaz
Copy link
Collaborator

No pressure or rush meant! Just checking, I was having a look at updating my summary in #44 and wasn't sure about this one. Nice!

@jeswr jeswr closed this Dec 1, 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.

2 participants