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 ReadStateChannel into a monad #62

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

make ReadStateChannel into a monad #62

wants to merge 2 commits into from

Conversation

fkz
Copy link

@fkz fkz commented Apr 26, 2016

I. e. add a flatMap/map to ReadStateChannel that also return a ReadStateChannel instead of just a ReadChannel.

I'm not sure if this is the right solution performance-wise because currently cache is used (so get is called each time we use this flatMap when my understanding is right). This can probably be done better with a more "lazy" approach.

Also, because of this overloading, flatMap(x => code) doesn't work anymore because scala is not able to recognize the type of x (because there are two flatMap's), you can see the necessary change in the diff.

I'd be happy to get feedback of the general idea/specific implementation.

@darkfrog26
Copy link
Contributor

This seems like a reasonable feature. @tindzk do you have any objections?

@tindzk
Copy link
Owner

tindzk commented Aug 4, 2016

Thanks a lot for the pull request and sorry for not looking into this earlier. I absolutely agree with you that ReadStateChannel should be a monad, but there are some problems with the solution provided:

  • asInstanceOf should not be needed
  • map and flatMap may have a considerable overhead

I will think more about it and try to come up with an alternative solution.

On a similar note, I have been thinking about depending on Cats. It would allow us to check that the monad laws are satisfied (see also http://eed3si9n.com/herding-cats/Monad.html).

@fkz fkz closed this Aug 7, 2016
@fkz fkz reopened this Aug 7, 2016
@fkz
Copy link
Author

fkz commented Aug 7, 2016

removed the asInstanceOf. The other points remain valid though.

@fkz
Copy link
Author

fkz commented Aug 7, 2016

Using cats would also solve the problem that flatMap (x => code) can be used again since we won't have two flatMap functions anymore.

Checking laws with cats might also be nice; beware that you have to provide Arbitrary and Eq instances for ReadChannel/ReadStateChannel for this though, so you have to be able to generate arbitrary instances and compare equality of them.
I did implement this with having a few base variables and then generate channels that depend on them (for the Arbitrary call) and for the equality change these base variables and see if the channels are emitting the same events. But a problem with this is that there are now a lot of channels depending on these base variables (from all the tests) and so the equality check gets slower and slower because all the unrelated channels have too be updated, too.

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