search

dimitri-xyz / haskell-coinbase-pro

A Haskell client for the Coinbase Pro API
MIT License
11 stars 6 forks source link

Remove ResourceT and move to LTS-15.12 #19

Closed dimitri-xyz closed 4 years ago

dimitri-xyz commented 4 years ago

The API is using ExceptT on top of IO. This is not a good idea as it makes the error reporting ambiguous. We should fix this.

At the same time, we were using ResourceT and that now depends on MonadUnliftIO which was failing the build. Our REST API calls do not return large bodies. So, I simply removed ResourceT. This allowed for an easier transition to the newer stackage LTS version.

The library now build on the latest stackage LTS-15.12 Testing on cabal still to be done.

ericpashman commented 4 years ago

I'm ambivalent about the "ExceptT-over-IO is an antipattern" mantra, feeling that ExceptT is fine for calling out common domain-specific error cases that should be handled as soon as possible. But the minimal set of errors defined in the current codebase don't seem terribly informative, and anyway I'm happy to try another approach. What do you have in mind?

I looked over this PR briefly. Looks good, I'll review properly later today, hopefully.

Does updating to the latest Stack LTS really not require any further changes? If so, we might want to merge this before my old PR, as I think there are a few things related to those changes that were awkward with the old dependency set that I may be able to polish up after rebasing on this. (I just have to remember what those things were, six months later. ...)

dimitri-xyz commented 4 years ago

I don't know what happened here. It seems something went wrong when... "and then a merge was required for some reason before pushing back to your branch". I --forced a cleanup.