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 foldMap/compile FunctionK to Free companion #1411

Merged
merged 2 commits into from
Oct 26, 2016
Merged

Conversation

johnynek
Copy link
Contributor

This came up for me in Free usage. We actually didn't want to call foldMap but to build another FunctionK.

Note, an interesting corollary to noting that Free converts a FunctionK[S, M] => FunctionK[Free[S, ?], M] and noting that we always have via liftF a FunctionK[M, Free[M, ?]] then if you have some code that requires complex recursive flatmaps, you can always first convert everything to Free via liftF, then in the end use the Monad.tailRecM along with foldMap(FunctionK.id). So, in it seems that many classes of recursion, as long as you don't change the Monad, are covered by implementing tailRecM.

@codecov-io
Copy link

codecov-io commented Oct 18, 2016

Current coverage is 92.01% (diff: 90.00%)

Merging #1411 into master will increase coverage by 0.33%

@@             master      #1411   diff @@
==========================================
  Files           240        242     +2   
  Lines          3608       3820   +212   
  Methods        3540       3751   +211   
  Messages          0          0          
  Branches         67         69     +2   
==========================================
+ Hits           3308       3515   +207   
- Misses          300        305     +5   
  Partials          0          0          

Powered by Codecov. Last update 86491ba...7798939

@travisbrown travisbrown mentioned this pull request Oct 18, 2016
13 tasks
@travisbrown
Copy link
Contributor

👍, but the names seem potentially a little confusing given that they reverse the usual order of arguments. I'm not sure that something like compileWith or foldMapWith is any better, though.

@non
Copy link
Contributor

non commented Oct 25, 2016

This seems basically good to me. Two questions though:

  1. Is there a reason to define Free.compile but FreeT.interpret? These seem like they could be the same name (and either name seems OK to me).
  2. Is there a reason to use FunctionK[F, G] in Free, but S ~> T in FreeT? This makes the methods seem more different than they really are.

@johnynek
Copy link
Contributor Author

@non yeah, when adding these I noticed we were somewhat inconsistent in Free and FreeT.

I made FreeT use the standard in the FreeT file. Should I add a duplicate name or rename interpret to compile in FreeT? If we want to make such a change, sooner is better, right?

@non
Copy link
Contributor

non commented Oct 25, 2016

Yeah for now let's add a duplicate name, deprecate the non-standard one, and call it a day.

* Use FunctionK instead of ~> alias in FreeT

* Rename interpret in FreeT to compile for consistency with Free
@non
Copy link
Contributor

non commented Oct 26, 2016

👍 once things are green

@non
Copy link
Contributor

non commented Oct 26, 2016

FWIW -- I'm fine with merging this despite not having a test to cover the deprecated method.

@johnynek johnynek merged commit 08b72eb into master Oct 26, 2016
@johnynek johnynek deleted the oscar/functionk-free branch October 26, 2016 04:39
@stew stew removed the in progress label Oct 26, 2016
@travisbrown
Copy link
Contributor

@non Yeah, we could define the new one in terms of the old, but then that's just another thing to do when we remove it. Having it uncovered is also a good motivation to get rid of it as soon as possible, too. :)

@johnynek
Copy link
Contributor Author

Call me reckless but a deprecated method that just redirects to another is
not my top anxiety about code coverage.
On Tue, Oct 25, 2016 at 18:50 Travis Brown notifications@github.com wrote:

@non https://github.com/non Yeah, we could define the new one in terms
of the old, but then that's just another thing to do when we remove it.
Having it uncovered is also a good motivation to get rid of it as soon as
possible, too. :)


You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub
#1411 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEJdiLTygGnMkdKaTbEyt1xWSXWEJwhks5q3twYgaJpZM4KaVdE
.

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.

6 participants