search

derekdowling / jsh-api

Golang JSON Specification API Inspired By JSH
MIT License
15 stars 3 forks source link

Customizable Sender #18

Closed derekdowling closed 8 years ago

derekdowling commented 8 years ago

In response to the need for the ability to use custom response send and log logic, I've defined a SendHandler function that allows people to swap in custom sender functions which JSHAPI will leverage.

A default sender is still provided and used by default. Also added an enhancement to go-json-spec-handler to further simplify status checking for sendable ErrorTypes.

derekdowling commented 8 years ago

Created in response to outcome of #17. @petercgrant do you have any thoughts or feedback on this?

petercgrant commented 8 years ago

TL;DR: It works for me. This is less opinionated than #17 but will still enable custom logging.

It seems a little strange that Logger is used only by SendHandler and jshapi.New. If you create an API using jshapi.New but use a custom SendHandler for the purpose of logging, you would still have the logging middleware added to goji, tied to a logger you don't use otherwise. You may have noticed I avoided that in #17 because I needed context setup middleware in the chain before logging. There may not be a good alternative that is backward-compatible, so I'm happy to use this as is.

derekdowling commented 8 years ago

Okay, I totally agree with what you've said and I think it comes down to a poor design decision early on. This is going to break backwards compatibility, but I am fine with that since I'm still striving towards a clean API rather than stability at this point. I've decoupled the Goji middleware and SendHandler setup from the default New() constructor for cases like yours where you'd like to setup those things yourself and created a Default() constructor that does all of those other things out of the box which as a side-effect removes eliminates the "sort of used" logger smell that you identified.

Overall, I think this is much cleaner than what I had before. The global SendHandler isn't my favorite but I want to avoid coupling the API and Resource types together since it reduces testability, and IMO is an anti-pattern. The good news is that these function definitions have changed so it will totally break when people update rather than sneakily changing the interface that they're expecting.

@petercgrant let me know what you think at this point as I'd appreciate a second set of eyes again.

petercgrant commented 8 years ago

LGTM. Thank you for taking the time to put this together!

derekdowling commented 8 years ago

Okay, I will merge and bump the version. Please open an issue if you see anything strange after trying this out. Feel free to update your other PR to add the leveled SendHandler if you'd like to contribute that still as one of the out of the box options people can select.