Make it easier to define more HTTP methods

[Porges]

Sending this for a bit of discussion.

This change factors out common functionality from the HasServer instances for
the various existing HTTP methods, so that there is only one real
datatype (HttpMethod) and the existing implementations are all just type
aliases for this.

This should hopefully make it near-trivial to define new HTTP methods.

I think this should be mostly source-compatible with the previous implementation?

Probably need to do something about the 'default' status codes as well (per #189).

[Porges]

Caution: I haven't actually tried to use this yet (stack build had some issues in the other projects about disabled components) so it's possible the entire idea is unworkable.

[jkarni]

The doctest should say Delete <content-type> () (our mistake - only now are the doctests failing here because previously it'd still compile)

[Porges]

Yes, I got quite confused starting out because the documentation says you can use Delete without arguments in a few places.

[jkarni]

Great idea, by the way. Thanks!

[Porges]

Had a think about how to achieve #189 :

1. Drop the default status code from the HttpMethod definition, so it's just the method name.
2. Create a type family to map e.g. HttpMethod "GET" = 200, so we can preserve existing behaviour.
3. Create a new result type ServantSucc(ess) (to go with ServantErr) defined as something like data ServantSuccess r = ServantSuccess Status r. If this is returned from a handler then the corresponding status code is used (this will be another overlap for HasServer - need to see how this plays with headers etc).
4. Add some helpers like ok r = ServantSuccess ok200 r and created r = ServantSuccess created201 r. Then a handler looks something like handler _ = do { _; return $ created _ }

This should continue to have backwards compatibility while allowing more flexibility in status codes.

I'll write this up after work today and see how it goes.

[codedmart]

I haven't been following along but any change I feel should allow the user to provide a status code. Regardless of what the specs say in case someone wants to provide a custome status code they should be able to. Is that where this is going?

[Porges]

Just realized I referenced #190 (this PR) instead of #189 in that comment - fixed. But yes, that's the idea.

[jkarni]

Maybe some documentation saying you can use this to define your own status code response/method?

[jkarni]

Looks great.

This changes the behaviour of methods to not return 204 when the type is (), as was previously the case. This is a trade-off: on the one hand, the correct thing in those cases is almost always 204; on the other, there are many other cases (sum types, more particularly) where there should be a 204 but there isn't, and thus the behaviour is incomplete and a little surprising. Also, our previous overlapping instances approach made certain things ugly (exponential blow-up in the number of instances). I'm on the fence about that change, but slightly in favour of it (not being too opinionated about things seems good). @alpmestan what do you think?

Otherwise (and aside from the small comment), this looks good to me.

[alpmestan]

I'm really not sure about this. How wrong would it be to not give 204 when appropriate, but e.g 200 instead? Would it mess things up with external tools? Would it annoy people?

[Porges]

@jkarni the intention is to not remove special-casing 204 for () (and indeed, not to affect the behaviour of any existing code). The overlapping instances will still be there (but won't have n*m instances).

[jkarni]

I like this idea. Another option maybe worth discussing is just putting in a StateT in the midst of the stack with the status code (initialized to 200 for GET, 201 for POST, 204 for (), etc.). In theory I guess we could then even remove the EitherT then (and have users themselves declare sum types when they may return very different datatypes. That would be a pretty fundamental change, though.

[jkarni]

Hm, thinking more on it, I think we want to go in the direction of making HTTP status codes more rather than less knowable statically. (And the handlers less rather than more HTTP-aware.)

[jkarni]

@Porges I like the type synonyms, but I'm not so sure about ServantSuccess. Do you want to separate out the changes, so we can discuss them independently?

EDIT: Or make this PR just about the type synonyms, and then open an issue for discussion of ServantSuccess.

[Porges]

Sorry about lack of movement on this - my personal laptop deaded itself and I haven't replaced it yet...

[jkarni]

Closing in favour of #276 (and thanks!)