search

snowleopard / selective

Selective Applicative Functors: Declare Your Effects Statically, Select Which to Execute Dynamically
MIT License
202 stars 21 forks source link

Add custom ExceptT (#37) #39

Closed turion closed 2 years ago

turion commented 3 years ago

This implementation of ExceptT has an Applicative instance that does not need a Monad constraint of the underlying context.

turion commented 3 years ago

Do you want to have MaybeT treated in the same way? It would make sense, I guess.

snowleopard commented 3 years ago

Do you want to have MaybeT treated in the same way? It would make sense, I guess.

That sounds good but it may be best to finish the ExceptT story first.

Please ping me when this PR is good to review!

turion commented 3 years ago

Do you want to have MaybeT treated in the same way? It would make sense, I guess.

That sounds good but it may be best to finish the ExceptT story first.

Yes, agree. I wanted to upload this first stub so you can see the direction we're heading.

snowleopard commented 3 years ago

Do you want to have MaybeT treated in the same way? It would make sense, I guess.

That sounds good but it may be best to finish the ExceptT story first.

Yes, agree. I wanted to upload this first stub so you can see the direction we're heading.

My expectation is that MaybeT will be just like ExceptT with e = (), but sure feel free to share the first stub if you like!

turion commented 3 years ago

Do you want to have MaybeT treated in the same way? It would make sense, I guess.

That sounds good but it may be best to finish the ExceptT story first.

Yes, agree. I wanted to upload this first stub so you can see the direction we're heading.

My expectation is that MaybeT will be just like ExceptT with e = (), but sure feel free to share the first stub if you like!

Oh yes, I believe that as well. I rather meant that I wanted to share this PR here with this one file before I go on working everything out, since it contains the essential motivation for doing all that work.

snowleopard commented 3 years ago

Sure, sounds good!

turion commented 2 years ago

I don't know why I didn't mark it as ready for review. I believe it is.

turion commented 2 years ago

Ah, deriving MonadFail of course doesn't work for older GHCs. What's your policy for these situations? Deprecate older GHCs? Don't derive MonadFail? CPP?

snowleopard commented 2 years ago

Ah, deriving MonadFail of course doesn't work for older GHCs. What's your policy for these situations? Deprecate older GHCs? Don't derive MonadFail? CPP?

If it's just about MonadFail, then let's just write the instance by hand.

snowleopard commented 2 years ago

Could you test this transformer somehow? We do have a little testsuite for checking all Selective laws of various instances.

turion commented 2 years ago

Could you test this transformer somehow? We do have a little testsuite for checking all Selective laws of various instances.

Yes, I'd replicate the Writer test suite and adapt it.

But there is a problem: Where to put the Arbitrary instance for ExceptT? Putting it in the tests is an orphan instance. Also we would probably like to make the instance public so others can use it in their tests as well. But we don't want QuickCheck as a dependency to the main library.

I know two typical solutions to this situation:

I like the second option more.

turion commented 2 years ago

Ah, we have to add an orphan instance for transformers' ExceptT anyways.

turion commented 2 years ago

I went for the first option for the time being. Are there any extra laws that make sense checking for ExceptT?

turion commented 2 years ago

It becomes more cumbersome to derive instances:

Again the question what to do. Some options:

  1. Drop older GHCs
  2. CPP
  3. Don't add instances for MonadFail and Contravariant

I personally like 1. by far the most. But of course the choice is yours.

snowleopard commented 2 years ago

@turion Apologies for the long silence! I somehow missed the notifications.

While I don't mind dropping compatibility with older GHCs in principle, doing this just to support MonadFail and Contravariant instances doesn't seem great. Let's just omit these instances for now. I doubt anyone will notice, to be honest :)

snowleopard commented 2 years ago

Are there any extra laws that make sense checking for ExceptT?

I'm not aware of any.

snowleopard commented 2 years ago

Orphan instance in tests

Yes, this is fine. Thanks!

turion commented 2 years ago

Ok, then I think I've done everything :)

turion commented 2 years ago

The build failure is caused because except used to have the type Either e a -> Except e a which changed to Either e a -> ExceptT e m a at some point. Older GHC versions somehow require older transformers versions indirectly. Again I'm not sure how to proceed. We can do CPP to have two definitions depending on GHC versions, drop Control.Selective.Trans.Except.except, or drop older GHCs.

snowleopard commented 2 years ago

The build failure is caused because except used to have the type Either e a -> Except e a which changed to Either e a -> ExceptT e m a at some point.

Ah, that's unfortunate. Let's use CPP to match the type for older GHC versions.

turion commented 2 years ago

If we already have CPP, shall I re-add MonadFail and Data.Functor.Contravariant?

snowleopard commented 2 years ago

If we already have CPP, shall I re-add MonadFail and Data.Functor.Contravariant?

Sure, let's do that. Thanks!

snowleopard commented 2 years ago

If we already have CPP, shall I re-add MonadFail and Data.Functor.Contravariant?

Sure, let's do that. Thanks!

Looks like it may be harder than it seemed. Feel free to skip this bit if you like!

turion commented 2 years ago

@snowleopard I could figure it out in the end. Ok like this? Then I'll squash.

snowleopard commented 2 years ago

Awesome, many thanks! I've squashed the commits.

I think it's a pretty cool new module :) I'll try to release a new version of the library soon.

turion commented 2 years ago

Awesome, this was great fun :)

turion commented 2 years ago

I just realised (again?) that the ExceptT instance is just the one from composing with a Traversable https://github.com/snowleopard/selective/issues/12#issuecomment-884793429 I believe. So in the long run I think we should define another newtype plus instance for the general case and derive Selective that way.

turion commented 2 years ago

Also, was it really necessary to delete the old transformers instance? It's more restrictive than this one, but still it doesn't do any harm, right?

snowleopard commented 2 years ago

Agreed with both points! It would be cleaner to reuse a general Traversable based implementation in the ExceptT instance. And the old instance can remain, perhaps with a comment pointing to the existence of a less restrictive instance.