search

haskellari / postgresql-simple

Mid-level client library for accessing PostgreSQL from Haskell
Other
84 stars 43 forks source link

feat: add superclass for all postgresql exceptions #108

Closed mrcjkb closed 11 months ago

mrcjkb commented 1 year ago

This adds a common superclass SomePostgreSqlException for all four Exception types:

The benefit is that clients can call fromException to handle all postgresql-simple exceptions (see isPostgreSqlException in test/Exception.hs), rather than being forced to handle all SomeExceptions.

Without SomePostgreSqlException, clients have to check whether a caught SomeException is one of the 4 postgresql-simple exceptions. This is error-prone, as the addition of a new type of exception to this library would go unnoticed.

This change is backward-compatible.

Note: I ran stylish-haskell, but it formatted many files that I hadn't touched. So instead, I manually formatted my changes.

phadej commented 1 year ago

Pre GHC-8 will fail. But maybe it's time to drop support for them...

mrcjkb commented 1 year ago

Pre GHC-8 will fail. But maybe it's time to drop support for them...

Looks like the failure was because of a redundant pragma (left over from an earlier implementation). If it works pre GHC-8, I don't think this is a breaking change.

phadej commented 1 year ago

don't think this is a breaking change.

The proof obligation is on you. As maintainer I have to be conservative in unclear cases.

phadej commented 1 year ago

Please tell when you are reaedy for the review (i.e. stop force pushing).

mrcjkb commented 1 year ago

Please tell when you are reaedy for the review (i.e. stop force pushing).

Sorry, I somehow missed the QueryError instance. It turns out cabal was only running the inspection suite and printing "PASS" for the tests suite without actually running any tests when I ran it locally.

mrcjkb commented 1 year ago

Marking as draft again, because I would like to add a test to prove that you can still catch the individual exceptions

mrcjkb commented 1 year ago

This should be ready now.

mrcjkb commented 1 year ago

Seems mostly OK. This is a breaking change, so i'll take my time thinking this through.

@phadej have you had time to think it through? Sorry for pinging... This PR has been open for a while and I wanted to make sure it hasn't been forgotten :)

phadej commented 1 year ago

I'm planning to do major postgresql-simple release after

Then I can remove Eq Null instance and include this. This is technically a breaking change (though hopefully not in practice), so I'll include it then.

mrcjkb commented 1 year ago

Thanks for the info + review!

phadej commented 11 months ago

Merged in https://github.com/haskellari/postgresql-simple/pull/124