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

Add transducer protocol to Immutable Data Structures #550

Closed
wants to merge 3 commits into from

Conversation

donabrams
Copy link
Contributor

Completed as per cognitect-labs/transducers-js#20. cc: #538.

I ran some tests locally (see https://gist.github.com/donabrams/3f42e7017170b52593f5), but I was using an implementation (https://github.com/transduce/transduce) to test this rather than some standalone code. I'm not sure what tests would be appropriate to commit here.

@leebyron
Copy link
Collaborator

Hey @donabrams let's update this and get it working!

It looks like the transducer protocol suggests that it use symbols when available, should this do the same?

You can look at how Immutable implements the Iterator protocol for an example.

@donabrams
Copy link
Contributor Author

It looks like the double at strings were used at the time, since symbols weren't ready yet. Only Gonzala used symbols:
Symbol.for("transducer/step") vs. "@@transducer/step"

@leebyron
Copy link
Collaborator

Ok, we can omit Symbols for now and add them later. This is cool.

Could you please include some simple test cases? That will help ensure these never break in the future, but would also document the minimal use cases.

@donabrams
Copy link
Contributor Author

Yeah, I originally drew a blank when deciding what tests I'd write for this. Since these interfaces are declarative but imply that these data structures hold certain properties, I'd think the right test would be around the property invariants. Really, there should be a generative/invariant test for anyone implementing transducer protocol but I couldn't find one :( I'll update this w/in the next few days.

@ghost ghost added the CLA Signed label Jul 12, 2016
@keeth
Copy link

keeth commented Jul 25, 2016

Would love to see this merged.. anything an outsider could do to help move it along?

@lacker
Copy link

lacker commented Dec 6, 2016

This pull request seems like it has gotten stalled in "needs tests" state, and I theorize it is stuck there indefinitely, so I am going to close it. If someone is interested in working on this then I think it could definitely still be a useful area to work on though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants