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

Make buffer implementation pluggable #9

Merged
merged 1 commit into from
May 10, 2018
Merged

Conversation

edescourtis
Copy link
Contributor

NOTE: This is a work in progress intended only for review.

How does something like this look?

@ferd
Copy link
Owner

ferd commented May 4, 2018

Yeah that sounds good to me. Plus we can probably test it easy by making a sample buffer that's just the queue module and ensuring it works like the existing queue option

@edescourtis
Copy link
Contributor Author

edescourtis commented May 4, 2018

Should we allow a buffer to receive options in the new function?

@ferd
Copy link
Owner

ferd commented May 4, 2018

My initial reaction would be to say no because I'd believe things need to be simple, but I guess {mod, Mod, Args} could potentially work. It just means that the default buffers couldn't be implemented without a shim because they don't take arguments, so that's not a 100% match between semantics I guess.

@edescourtis
Copy link
Contributor Author

Okay, I won't include it in this version but we can always add it later.

See samples/pobox_queue_buf.erl for an example pobox_buf implementation.
@edescourtis
Copy link
Contributor Author

Take a look at this commit. Tell me if it's okay with you.

@ferd
Copy link
Owner

ferd commented May 4, 2018

Cool, I'll try to take some time for this around the week-end. I've got a lot of OTP-21 prepardness to do first, but that should be on my list as well.

@edescourtis
Copy link
Contributor Author

Let me know if you need help with anything.

@ferd ferd merged commit 7ddeb97 into ferd:master May 10, 2018
@ferd
Copy link
Owner

ferd commented May 10, 2018

Merged, re-versionned to 1.1.0, and published to hex + tagged. Thanks for the contribution!

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.

None yet

2 participants