Conversation
this.coercionConventions = coercionConventions; | ||
} | ||
|
||
public Response NegotiateResponse(object result, NancyContext context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is very long. I'd do some extract methods. A few suggestions for where below.
Sure, by all means. Some of the methods could be refactored. They're just a clean rip-out from the existing conneg stuff from |
Alright @horsdal, what do you think now? I struggled a bit to grasp what the existing methods tried to accomplish, so I factored them a bit differently and added XML comments so anyone should be able to understand the basics of what's going on. Here's a map of the method calls: I also took the liberty of replacing the |
@khellang Much nicer factoring! The methods make a whole lot more sense to me now. |
I actually find |
+1 |
OK, so this is most of the change I've been wanting to get in for a while (in #1213). It's basically ripping out the conneg parts from the
DefaultRouteInvoker
and putting them in a separate serviceIResponseNegotiator
with it's default implementationDefaultResponseNegotiator
.I'd refactor (move) the tests as well, but it seems there's no unit tests for the conneg parts and 92% of the new response negotiator is covered by functional tests (the rest is trace calls).
The next step (for a future PR) would be to use this service to do conneg in other pipelines 馃槃