silkapp / rest

Packages for defining APIs, running them, generating client code and documentation.
http://silkapp.github.io/rest
390 stars 52 forks source link

bindReason #108

Closed bergmark closed 9 years ago

bergmark commented 9 years ago

How about including this?

bindReason :: (a -> Reason a) -> Reason a -> Reason a
bindReason f = \case
  CustomReason (DomainReason a) -> f a
  x                             -> x

Edit: or more bindingly and useful (needs more uniplate):

bindReason :: (e -> Reason f) -> Reason e -> Reason f
bindReason f = \case
  CustomReason (DomainReason a) -> f a
  NotFound                      -> NotFound
  UnsupportedRoute              -> UnsupportedRoute
  UnsupportedMethod             -> UnsupportedMethod
  UnsupportedVersion            -> UnsupportedVersion
  NotAllowed                    -> NotAllowed
  AuthenticationFailed          -> AuthenticationFailed
  Busy                          -> Busy
  Gone                          -> Gone
  InputError  d                 -> InputError d
  OutputError d                 -> OutputError d
  IdentError  d                 -> IdentError d
  HeaderError d                 -> HeaderError d
  ParamError  d                 -> ParamError d
bergmark commented 9 years ago

And while I'm at it :-) Is there a reason for the ToResponseCode constraint on domainReason? I have one case where it is practical to temporarily stick another type in there, which I can work around by using the constructors explicitly.

domainReason :: ToResponseCode a => a -> Reason a
hesselink commented 9 years ago

Why not do an actual Monad instance? You'd have to pattern match on all constructors, since the type is changing, but this seems fine to me:

instance Monad Reason where
  return a = CustomReason (DomainReason a)
  r >>= f = case r of
    CustomReason (DomainReason a) -> f a
    UnsupportedRoute              -> UnsupportedRoute
    UnsupportedMethod             -> UnsupportedMethod
    UnsupportedVersion            -> UnsupportedVersion
    IdentError   e                -> IdentError   e
    HeaderError  e                -> HeaderError  e
    ParamError   e                -> ParamError   e
    InputError   e                -> InputError   e
    OutputError  e                -> OutputError  e
    NotFound                      -> NotFound
    NotAllowed                    -> NotAllowed
    AuthenticationFailed          -> AuthenticationFailed
    Busy                          -> Busy
    Gone                          -> Gone

Regarding the second one, it doesn't seem to be needed. You actually added this :) in 50f4c732, and it didn't seem to be needed then either. I don't see any downside to just removing it.

bergmark commented 9 years ago

I actually tried the Monad instance but thought the applicative instance was too weird :) But sure!

hesselink commented 9 years ago

What's weird about the applicative instance?

bergmark commented 9 years ago

Eh I'm probably just tired :)

And i need to git blame more! Seems like we can just remove the constraint then. I can open a PR that we can merge before the next major release.

hesselink commented 9 years ago

I don't think this needs to go in a major release. I can't think of any way removing the constraint will break anything, and adding non-orphan instances is fine in a minor release now, right?

bergmark commented 9 years ago

Adding the instance is fine for sure.

I'm trying to think whether removing a constraint could break anything... but I don't have an example.

hesselink commented 9 years ago

It's making a signature more general, so I can't see it breaking anything. Generalizing something from e.g. Int -> [Int] to a -> [a] could break inference somewhere because it's more polymorphic, but that also doesn't happen here.

hesselink commented 9 years ago

Do you want me to add the Monad instance BTW, or do you already have it lying around?

bergmark commented 9 years ago

Please go ahead, I threw it out.

hesselink commented 9 years ago

Committed as 50c93134065c31f7e56b20092b1f4512a00bd557, will release as rest-core-0.35.1.