Bind dynamic date-format var for custom encoders #158
Conversation
| (get (:impls JSONable) (class obj)) (#'to-json obj jg) | ||
|
|
||
| (get (:impls JSONable) (class obj)) | ||
| (binding [*date-format* date-format] |
There was a problem hiding this comment.
I think the binding should be done at a higher level, in core.clj rather than here, since that is where it's passed in. What do you think?
There was a problem hiding this comment.
I'm not sure I see a reason to expose the details of the *date-format* var to namespaces outside of cheshire.generate. I chose to set the binding where I did so that the scope of the binding is as narrow as possible. Right now the only contexts I see that have reason to consume the value of *date-format* are the client-defined JSONable encoding functions, so the location the binding is set in my suggested version limits the binding to only that context. Keeping the scope of the binding narrow minimizes unnecessary coupling to other namespaces and functions and will keep the design more modular.
There was a problem hiding this comment.
@dakrone Any further thoughts on this PR? It may not matter much how this is implemented as long as the caller can do something like this and it works:
;; Somewhere in configuration code
(gen/add-encoder DateTime
(fn [^DateTime dt ^JsonGenerator jg]
(gen/encode-date (.toDate dt) jg))
;; Somewhere in application event handler
(json/encode {:date (org.joda.time.DateTime/now)}
{:date-format "yyyy-MM-dd"})
#120
#120 (comment)