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 CofreeT #26

Merged
merged 8 commits into from
Dec 10, 2020
Merged

Conversation

eviefp
Copy link
Member

@eviefp eviefp commented Sep 20, 2020

Description of the change

I'm looking for early feedback.

Here's my attempt at implementing CofreeT. I'm not very sure it's correct, so I would love some feedback. I've mostly followed the existing FreeT code and looked at the Haskell CofreeT signatures.

Fixes #14

I will definitely add documentation, and follow the rest of the checklist as appropriate. I'm currently looking for early feedback on the implementation. If it would make it easier for you to provide feedback on a "complete" PR (i.e. including docs, tests, etc.), I would be happy to oblige.


Checklist:

  • Added the change to the changelog's "Unreleased" section with a link to this PR and your username
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation in the README and/or documentation directory
  • Added a test for the contribution (if applicable)

@JordanMartinez
Copy link
Contributor

I'm not familiar enough with CofreeT to comment on this. Overall, it looks like the initial PR is good. My guess is that we're not looking for the most performant implementation of this, or even the most complete implementation; merely, an implementation in the first place, correct?

@eviefp
Copy link
Member Author

eviefp commented Sep 22, 2020

Alright, thank you for the feedback. I'll proceed to add docs, tests, etc., as to make it ready for review/merge!

@eviefp
Copy link
Member Author

eviefp commented Sep 22, 2020

I have no idea how to write unit tests for CofreeT since all the functions seem to only have one implementation which type-checks. I think the only "interesting" choice in the module is actually the type itself.

Note that, despite me having added a file under test/, it's not actually a test: it's just an example.

I think this is now ready for review!

@JordanMartinez
Copy link
Contributor

LGTM. The docs helped me understand why I would use this, so thanks!

@thomashoneyman
Copy link
Contributor

I'll have time to review this certainly by the end of the week. Thanks for the PR!

@eviefp
Copy link
Member Author

eviefp commented Sep 23, 2020

I realised I might want to do one more thing: consider what other instances I could add to CofreeT. I think there's quite a few. I'll have them ready before the weekend :)

EDIT: nevermind, they are done 👇

Copy link
Contributor

@thomashoneyman thomashoneyman left a comment

Choose a reason for hiding this comment

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

I'm not particularly familiar with Cofree, so it would be nice to have another look from @garyb (if you have a moment or suggestions for anyone else who should verify this). That said, it looks pretty good to me! Just a couple of suggestions.

src/Control/Monad/Cofree/Trans.purs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/Control/Monad/Cofree/Trans.purs Outdated Show resolved Hide resolved
src/Control/Monad/Cofree/Trans.purs Outdated Show resolved Hide resolved
src/Control/Monad/Cofree/Trans.purs Outdated Show resolved Hide resolved
src/Control/Monad/Cofree/Trans.purs Outdated Show resolved Hide resolved
src/Control/Monad/Cofree/Trans.purs Outdated Show resolved Hide resolved
@thomashoneyman
Copy link
Contributor

There is also this implementation which relies on Lazy (though yours via Unit -> ... accomplishes the same thing), which you may find interesting to compare:

https://github.com/arthurxavierx/purescript-comonad-ui-todos/blob/master/src/Control/Comonad/Cofree/Trans.purs

@eviefp
Copy link
Member Author

eviefp commented Sep 29, 2020

I addressed all review comments.

About the Lazy vs Unit -> bit, the only reason I would keep it as-is is because we're not already depending on lazy. But if you think it's better to add that dependency, it's a trivial change. I haven't used lazy much (I even forgot it exists) -- that's the only reason I didn't implement it like that from the beginning.

@thomashoneyman
Copy link
Contributor

No need to introduce it, although since we’re already bringing in new dependencies it wouldn’t be that big of a deal.

I’d like to let this sit for a week or so for others to weigh in, but if no one does by then I will go ahead and merge because it appears correct to me (with the caveat that I’m not particularly familiar with this construct).

instance comonadCofreeT :: (Comonad m, Functor f) => Comonad (CofreeT f m) where
extract = extract <<< head

instance eqCofreeT :: Eq (m (Tuple a (f (CofreeT f m a)))) => Eq (CofreeT f m a) where
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't be guaranteed to terminate in most actual use cases. Is there a reason for adding it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it because we expect CofreeTs to generally be infinite? The main reason I added it is because this instance exists in Haskell as well, but I see your point. I'm not sure this would get a lot of (legitimate) use. Maybe it's better to remove and consider re-adding if we get requests/issues for real use cases.

Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to use Eq1 m instead of this constraint, if we keep it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed both Eq and Ord for now. It might be possible to use Eq1 though, so I'll keep that in mind if we end up implementing it eventually.

@eviefp
Copy link
Member Author

eviefp commented Oct 1, 2020

I think I addressed all review comments. Thank you for taking your time to review my PR!

@thomashoneyman thomashoneyman merged commit bfb105e into purescript-contrib:main Dec 10, 2020
@eviefp eviefp deleted the feature--cofreet branch December 12, 2020 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Include CofreeT?
5 participants