add Enumerator.ensureEval

[johnynek]

Addresses #105

[johnynek]

If this looks basically sane, I'll add a test of say a FileModule[Try], which is clearly strict and fails without this.

[johnynek]

looks like a linter error. I'll fix if you think this approach looks okay.

[travisbrown]

Thanks, @johnynek!

[travisbrown]

Will merge when green (it does look like it's just a line-length issue, although I'm not sure why 2.11 is showing up as passing…).

[johnynek]

Actually, as you can see here, laziness in captureEffect is not enough. We also assume that recursive flatMap is safe.

I think we should put this on pause, but I am leaving it here. To really make this work, I think we need cats 0.7.0 and replace the flatMap with tailRecM, and then we expect a MonadError[M, Throwable] with MonadRec[M]. At that point, using Xor, Try or a raw Future could work in these cases.

[travisbrown]

@johnynek Oh, makes sense. I have a branch waiting for Cats 0.7.0 in #104 (although note that you'll have to publish the Cats 0.7.0 from catbird locally in order to run the full test suite).

[travisbrown]

@johnynek I've been thinking some more about this in the context of the related Cats issue, and I'm wondering how necessary you think it is to support stack safety with Try directly here.

It's just so much simpler to say that Iteratee[F, ?], etc. will be stack-safe if and only if F is (which is what I do say in the blog post introducing the project, although it's not as clear as it should be in the API docs), and that if your F isn't stack-safe and you need Iteratee[F, ?] to be, you'll need to lift it into Free[F, ?] or something similar.

[johnynek]

I don't think we should trampoline, but I think we should use tailRecM
calls.

As I'm sure you know, iteratee is the motivating example for me wanting to
get serious out tailRecM. The worst worlds outcome to me would be us
keeping MonadRec yet libraries like Iteratee still saying we will not
require a MonadRec just make sure your flatMap is safe.

I mean, if tailRecM is not suitable for Iteratee, who exactly is it useful
for?
On Thu, Aug 11, 2016 at 04:57 Travis Brown notifications@github.com wrote:

@johnynek https://github.com/johnynek I've been thinking some more
about this, and I'm wondering how necessary you think it is to support
stack safety with Try directly here.

It's just so much simpler to say that Iteratee[F, ?], etc. will be
stack-safe if and only if F is (which is what I do say in the blog post
introducing the project
https://meta.plasm.us/posts/2016/01/08/yet-another-iteratee-library/,
although it's not as clear as it should be in the API docs), and that if
your F isn't stack-safe and you need Iteratee[F, ?] to be, you'll need to
lift it into Free[F, ?] or something similar.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#106 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEJdgk4F9qPlAJ_qB6sEWAktfp4ZhMeks5qezhJgaJpZM4JftTK
.

[travisbrown]

@johnynek If Cats goes the tailRecM-in-Monad route, we'd follow suit here (assuming it doesn't compromise other principles of the project, and I don't see any reason to think that'd be the case).

I still don't really like it, though. To take an analogy—at some point someone opened an issue against circe where they were passing null as a Json argument to a method on Printer (or something like that) and getting an NPE. I see that as a 100% unambiguous wont-fix—there are lots of good ways to avoid null in Scala, and the point at which that should be done is way before you call a method in a JSON library. Similarly here—there are lots of monads in Scala where recursive binding is stack-safe, and for the ones where it's not, there's Free.

But I'm realizing that's not a popular position, and I'm happy to go along with what everyone else wants if necessary. :)

[johnynek]

In light of the solution being proposed (all Monad have tailRecM but advertise stack safety with a marker trait), I am happy for this library to not require that marker trait, but it would be nice to use tailRecM as much as possible, and to try to implement instances that are stack safe if we can here.

[johnynek]

okay, I'll update this with your latest fixes, and see if we can have a design where FileModule might not be needed (I think perhaps just using catchNonFatal on MonadError and in some other cases using Eval[T] we can be okay.

[travisbrown]

@johnynek Great, thanks! I'm going to go ahead and publish a new milestone, and I'd like to try to get 0.6.0 out as early as possible this week, but there's not a huge rush from my perspective.

[johnynek]

well, from my perspective, as we are adopting more of these libraries at work, and across several differently sized repos, version churn is a significant cost. So, the fewer times we have to change code the better. So,

[codecov-io]

Current coverage is 0.00% (diff: 0.00%)

No coverage report found for master at 0828a49.

Powered by Codecov. Last update 0828a49...9708e68

[johnynek]

would be interesting to benchmark this. My guess is a strict monad (like Option, Either or Try) could be faster for single threaded applications.

[travisbrown]

@johnynek What do you think about providing this operation via a new type class, so we could follow the pattern in iteratee-core, where all of the operations are available on a FileEnumerators object, and FileModule is just a syntactic convenience? The implementations would be more or less the same, and that approach would be likely to involve fewer API changes if Cats introduces a more general solution to the problem.

[johnynek]

So, as I have discussed on the thread here:

typelevel/cats#1316

I don't see why F.catchNonFatal(a) possibly coupled with Eval.always is not what we want in practice.

I feel like making a typeclass creates a veneer of rigor that is actually absent, and I'd frankly rather just stick to the mechanical style of Eval + MonadError, or Eval + Monad if it can't fail (but very few true effects can't fail).

If we have a typeclass here, we can certainly generate those from something in cats, but that feels more hypothetical to me.

Given that this only adds liftMEval which I think is useful to have for laziness if nothing else, and it leaves the existing modules as is, it seems a conservative approach.

[travisbrown]

@johnynek I just opened #116 with an example of what the type class would look like, but since it just builds on this PR I'll go ahead and merge this one in any case. Do you mind removing the pg files and rebasing? I'd also be happy to do that—just let me know.

[johnynek]

@travisbrown rather than rebase, can you just use github's squash-merge when you merge so I don't need to bother? That's easy, right?

[johnynek]

(the green button now gives you the option to squash).

[travisbrown]

Thanks, @johnynek—merging this now, will follow up with a PR with some name changes (for captureEffect, etc.) for clarity.

[johnynek]

sounds good. Thanks