-
Notifications
You must be signed in to change notification settings - Fork 295
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
Fix display of exception error messages, #1079 #1080
Fix display of exception error messages, #1079 #1080
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";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather have a derived Show
instance and a custom implementation of displayException
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s not super helpful if people do want to catch the error, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you serious, GHC? okay, this is fine then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha yeah that was more or less my reaction too
Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>
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
catch
to 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:
@since
declarations to the HaddockAfter submitting your PR: