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

Typeclass instances for Reader. #249

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dangerousben
Copy link
Contributor

  • Alternative
  • Defer
  • Monad
  • Monoid
  • Eq

Copy link
Contributor

@travisbrown travisbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

def defer[A](fa: => Reader[A]): Reader[A] = Reader.flatten(Reader.value(() => fa).map(_()))
}

private[util] trait ReaderMonoidK extends MonoidK[Reader] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, this and the two above are separate traits just for the sake of code organisation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly - I have a vague idea about trying to implement some of the cats-effect typeclasses for Reader (not actually sure if it's possible), and if so I'd want to be able to mix Defer and Monad in without MonoidK.

- Alternative
- Defer
- Monad
- Monoid
- Eq
@felixbr
Copy link
Contributor

felixbr commented Aug 24, 2020

There seems to be an infinite recursion for Reader somewhere. Is this reproducible locally or only on CI (I haven't checked yet)?

@dangerousben
Copy link
Contributor Author

There seems to be an infinite recursion for Reader somewhere. Is this reproducible locally or only on CI (I haven't checked yet)?

It's reproducible (although somehow it didn't show up before I submitted the PR). IIRC the problem is that it's impossible to implement a valid Eq instance for Reader because comparing streams requires reading and thus mutating them.

I think the other typeclass instances are (at least from a practical point of view) valid, but the laws can't be tested without a working Eq.

@dangerousben
Copy link
Contributor Author

the problem is that it's impossible to implement a valid Eq instance for Reader

It might of course be possible to implement an invalid one that works well enough for the tests, but I haven't figured out how so far.

@felixbr
Copy link
Contributor

felixbr commented Aug 24, 2020

the problem is that it's impossible to implement a valid Eq instance for Reader

It might of course be possible to implement an invalid one that works well enough for the tests, but I haven't figured out how so far.

For testing you could maybe drain the streams into local variables, compare them and reset the streams with the elements you drained. That is, of course, not a good idea in production, but maybe it's enough in the tests (with documentation why we have to do this).

edit: Some streaming implementation also allow copying/broadcasting a stream without mutating the "original" instance. I don't know if the Twitter ones have this, though.

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.

3 participants