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

Generalize the base monad of fold-like operations #195

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented Sep 30, 2016

Previously the fold-like operations were restricted to fold operations
in IO, greatly limiting their usefulness. Here we generalize them to
any MonadMask, provided by the widely-used exceptions library.

Resolves #9.

Previously the fold-like operations were restricted to fold operations
in `IO`, greatly limiting their usefulness. Here we generalize them to
any `MonadMask`, provided by the widely-used `exceptions` library.

Resolves lpsmith#9.
@bgamari
Copy link
Contributor Author

bgamari commented Oct 5, 2016

Ping.

@lpsmith
Copy link
Owner

lpsmith commented Oct 6, 2016

Ahh yes, sorry for not taking a peek at this a bit sooner.

Nothing too wrong with this patch in principle, but in practice, IIRC, the exceptions package has a problem (?) inherited from MonadCatchIO-transformers. (?)

In any case, I have finally come to the point of view that the monad-control/lifted-base approach is indeed probably better. I likely would accept this or a similiar pull request that makes the same decisions as the Snap Framework, version 1.0.

@bgamari
Copy link
Contributor Author

bgamari commented Oct 6, 2016

Nothing too wrong with this patch in principle, but in practice, IIRC, the exceptions package has a problem (?) inherited from MonadCatchIO-transformers. (?)

Can you be more specific? I never quite understood what the objection to exceptions was. It's widely used, much easier to write instances for than monad-control, and fills precisely the need that postgresql-simple faces (cleanup during exception handling).

Also prevents spurious rebuilds with current cabal versions.
@sopvop
Copy link
Contributor

sopvop commented Jan 19, 2017

The problem with exceptions is that MonadMask does not allow you to make instances for short-circuiting monads like ExceptT e m or Snap.

However, it is only relevant for withTransaction function in postgresq-simple, and as my experience with it shows - you would still have to provide specialized versions of withTransaction over your short-circuiting monads anyway. So that you can decide if you want to rollback or commit.

Edit: Above also applies to folds, with current api you can't exit from fold early without exceptions anyway.

And there is a valid criticism of MonadBaseControl being tricky. Writing instances for it is not that trivial either.

I think that going the exceptions route would be better for most of postgresql-simple users.

@BardurArantsson
Copy link
Contributor

BardurArantsson commented Apr 19, 2017

Any chance of this getting in?

I got the impression that the general problem with short-circuiting monads like ExceptT is that it's actually impossible to implement a proper bracket for such monads if you try to decompose into separate operations?

OTOH, I think I class like MonadBracket might work? Unfortunately, it seems MonadBracket hasn't actually been published as a separate library, though it seems to be included in foundation.

(FWIW, it seems to me that MonadBracket actually does the right thing at the semantic level and that it's not actually worth it trying to decompose into separate type classes.)

@centromere
Copy link

Hi. Is there any chance that this PR can be accepted?

@BardurArantsson
Copy link
Contributor

Just to add to my previous comment: Even given the downsides of MonadMask, I think it would still be valuable to have this since it would mean less boilerplate if you're already using e.g. ReaderT IO ... which seems to be popular. If a better solution than MonadMask comes along later, it would still be possible to switch to that.

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.

None yet

6 participants