Fix display of exception error messages, #1079#1080
Fix display of exception error messages, #1079#1080parsonsmatt merged 4 commits intoyesodweb:masterfrom
Conversation
Fixes yesodweb#1079. Running the test script from that issue now prints Migrating: ALTER TABLE "person" ALTER COLUMN "age" TYPE INT8 [Debug#SQL] ALTER TABLE "person" ALTER COLUMN "age" TYPE INT8; [] persistent-repro-exe: Database migration: manual intervention required. The unsafe actions are prefixed by '***' below: *** ALTER TABLE "person" DROP COLUMN "age";
parsonsmatt
left a comment
parsonsmatt
left a comment
There was a problem hiding this comment.
A few changes please - also this can be released with 2.11.0.0 which is unreleased so far.
persistent/ChangeLog.md
Outdated
| @@ -1,5 +1,9 @@ | |||
| # Changelog for persistent | |||
|
|
|||
| ## 2.11.1.0 | |||
There was a problem hiding this comment.
I've got to update the changelog to make it less confusing - can you put this in 2.11.0.0?
There was a problem hiding this comment.
Sure thing! I’ve seen people use a heading “unreleased changes” at the top of changelogs, which seems to work okay.
| where | ||
| displayMigration :: (Bool, Sql) -> String | ||
| displayMigration (True, s) = "*** " ++ unpack s ++ ";" | ||
| displayMigration (False, s) = " " ++ unpack s ++ ";" |
There was a problem hiding this comment.
I'd rather have a derived Show instance and a custom implementation of displayException.
There was a problem hiding this comment.
I tried that to start with, but unfortunately GHC uses Show, not displayException, when exiting due to an uncaught exception 🤷♂️ I’m not sure how else to handle it. Maybe the exception type could just be data UnsafeMigrationException = UnsafeMigrationException and we could instead print the human readable error message before throwing it?
There was a problem hiding this comment.
That’s not super helpful if people do want to catch the error, though.
There was a problem hiding this comment.
are you serious, GHC? okay, this is fine then
There was a problem hiding this comment.
haha yeah that was more or less my reaction too
Co-authored-by: Matt Parsons <[email protected]>
2 participants
Fixes #1079. Running the test script from that issue now prints
This is arguably a breaking change, since if people were catching this exception in their code before, they'll need to change their uses of
catchto catch this type. If #969 was previously released without a breaking-level version bump and nobody complained, though, this is probably okay to release without a breaking-level version bump too?Before submitting your PR, check that you've:
@sincedeclarations to the HaddockAfter submitting your PR: