Add typeclass for running ExceptT within a transaction

[natdash]

If ExceptT evaluates to a Left, transaction is rolled back.

[lykahb]

Thank you for the PR! This looks useful. Can you add transformers-compat to keep compatibility with GHC 7.8?

[natdash]

Sure! Would an example like this be helpful to include as well?

[lykahb]

An example would be helpful. Changing an existing example to use TryAction in every signature may portray it as the default idiom. A new example which shows it in one particular case, like inserting into a table which has a constraint, may be more obvious. If you don't have time for that, I can make an example myself.

Can you also add a unit test for the new code? Other than that, the PR looks good to merge.

[natdash]

Can do - I've updated the example in the same gist here. It uses a constraint to demonstrate catching a thrown exception, and also a contrived example where external logic may return an Either rather than throwing an exception. What are your thoughts?

No problem, I'll add the unit test as well.

[lykahb]

Thanks! That example shows all important things about the feature. It's a bit big, though. Ideally, an it should fit into the screen. The relationship is demonstrated in another example, so we don't have to repeat it here.

https://gist.github.com/natdash/1f4b8c922b979c413fdd786f1c255da6/dd03bbbf240ff0760cdb2da36dfd937d877287e3 is more condensed. I've made some changes to it https://gist.github.com/lykahb/148954be3ce8b5e260874109cdfcbe7b.

When editing, I noticed that it's hard to pass inline code into runTryDbConn as the exception parameter must be specified. It'll be nice to improve on that, but I have no concrete ideas.

[natdash]

Thanks for the condensed example! I'll include in the next commit.

Re inline code, do you think this would generally be used for catching thrown exceptions only, without also evaluating an ExceptT? I could include a helper function that does so for an Action, returning Either SomeException a without requiring an exception type parameter.

[lykahb]

I think a helper would be great. It can simplify the unit tests and some parts of the example. The tests look good, but the runner has an issue.

There are three test groups - one for each database. TryAction is not major enough to have a group on its own. The call to concatMap was just a convenient way to share the test suites across the databases, it's not an idiom that has to be to applied for each suite. The code can be simplified as in

#if WITH_POSTGRESQL
  let postgresql = [testGroup "Database.Groundhog.Postgresql" $ concatMap ($ runPSQL) [mkTestSuite, mkSqlTestSuite, postgresqlTestSuite] ++ mkTryTestSuite runPSQLWithConn]
      runPSQL m = withPostgresqlConn "dbname=test user=test password=test host=localhost" . runDbConn $ m >> cleanPostgresql
      runPSQLWithConn m = withPostgresqlConn "dbname=test user=test password=test host=localhost" $ \conn -> m conn
#else
  let postgresql = []
#endif

After fixing that and the Travis failure it should be good to merge.

[natdash]

Thanks for the feedback - I've updated the runner and fixed the Travis failure.

Re the helper, I've refactored and generalised tryWithConn to take a function for evaluating an action to an Either, so it can be extended to handle types other than ExceptT. I added a helper for trying Actions:

c <- runTryDbConn' (insert $ Customer "John Doe" "0123456789") conn 
-- c :: Either SomeException (Key Customer BackendSpecific)

[lykahb]

Thank you, that helper simplifies lots of code. Can you convert some calls in the example to runTryDbConn' with a comment that explains when runTryDbConn' or runTryDbConn are the best to use?

[natdash]

Sure, I've updated the example now.

[lykahb]

Thank you! Can you squash the commits? If I do that, the author field will be replaced.

[natdash]

Of course - done.