Add MonadError in addition to MonadThrow.

commercialhaskell / path

Typed filepath

Other

122 stars 45 forks source link

Add MonadError in addition to MonadThrow. #162

Closed sellout closed 3 years ago

sellout commented 4 years ago

This abstracts Include.hs to be able to swap between MonadError and MonadThrow. The MonadError implementations are exposed via Path.Except[.*], while MonadThrow remains in Path[.*].

Some shortcomings and considerations:

there is now a dependency on MTL
-Wunused-imports is disabled so both classes can be imported without having to conditionalize anything
there are no tests run against Path.Except[.*] yet

A slightly different approach could be to publish two packages -- path and path-except. They would expose the exact same modules (.Except would disappear), but one would have a dependency on exceptions and the other on mtl. It's unlikely that anyone would want to use both the MonadThrow and MonadError approaches in the same project, so this could reduce their dependency footprint, but it does mean I have to fix the unused imports.

Fixes #149.

NorfairKing commented 4 years ago

The lacking tests are a blocker for me.
I don't care about -Wunused-imports as long as there is no -Werror and -Wall is still on.
I would also be ok with a separate package. What do other maintainers think of this?

sellout commented 4 years ago

I think I would prefer the separate packages, personally. And yes, I will definitely get the tests working before this is merged, but I won’t get a chance until Tuesday and didn’t want to hold up any discussion.

NorfairKing commented 4 years ago

I think separate packages would also be easier for us. I'm just not sure how you would organise the code in that case, but I'll let you figure that out.

locallycompact commented 4 years ago

Did the new package happen? I'd quite like to try this.

sellout commented 4 years ago

Ah, sorry, I ended up dropping this, because our move to path is lower priority than other work. I forgot where I got to, but https://github.com/sellout/path/commit/f4267d74b075799d989c0ffbde6dbb4f33d9b9cf has changes on top of this PR that separate the two. NB: It's just one package, but it builds with mtl/MonadError instead if you do cabal build -f except.

I can turn that into a new PR, or whatever if someone wants to pick it up (it looks like the package compiles either way, but the tests don't quite yet -- should be easy to have the tests depend on both mtl and exceptions, since they're just tests, to minimize the conditionals/CPP required there).