dakrone / cheshire

Clojure JSON and JSON SMILE (binary json format) encoding/decoding
https://github.com/dakrone/cheshire
MIT License
1.49k stars 152 forks source link

feature: context specific encoders #77

Open noisesmith opened 9 years ago

noisesmith commented 9 years ago

I would like to make a structured logger with JSON output, but one catch would be that you might want a different output for a given object when logging as compared to other usage of JSON in the same application.

Is there an obvious way to implement this with the existing cheshire design?

Is this a feature that would be welcome for cheshire if I implemented it?

dakrone commented 9 years ago

Is there an obvious way to implement this with the existing cheshire design?

Not that I can immediately think of, I will have to experiment to see if there already is.

Is this a feature that would be welcome for cheshire if I implemented it?

Yes, definitely would be nice as long as it does not negatively affect performance.

gfredericks commented 9 years ago

This has been a big pain point for me as well.

One option I just thought of is to have another option that can be passed of a :fallback-encoder -- an encoder function that gets used whenever the JsonGenerationException would otherwise be thrown. Presumably that wouldn't be a big perf problem (at least for people not using it) since it's only a code change in an otherwise-exceptional codepath.

For my use cases I think the fallback encoder would pretty much always just use str, so we could also consider adding a feature that just does that, or at least an easier way to opt into that than an actual encoder fn.

gfredericks commented 9 years ago

Though now that I look through the implementation I remember that options are passed around as first-class args, and so having an extra option means expanding the signatures everywhere :/

gfredericks commented 9 years ago

If the signature thing is a big perf deal, maybe an internal dynamic var that gets set once at the top level would be an alternative.

dakrone commented 9 years ago

I remember that options are passed around as first-class args, and so having an extra option means expanding the signatures everywhere :/

I'd need to check the performance hit for it, but if it's not too much, a breaking change (Cheshire 6.x) to switch to a map of options would be better for future development.

gfredericks commented 9 years ago

I'll see if I can quickly make a branch with the options-map change.

dakrone commented 9 years ago

Okay, remember not to use destructuring as that (unfortunately) causes a big performance hit.

gfredericks commented 9 years ago

do you just mean destructuring when you don't actually need all the keys? When I macroexpand a {:keys [foo bar]} destructuring the only extra thing I see is a (seq? m) check, is that the bad part?

dakrone commented 9 years ago

I haven't dug into why it's slower yet, only that it was slower in benchmarks (heck, maybe Clojure has caught up and it's better in recent versions?), so it would be good to test and see.

gfredericks commented 9 years ago

okay; it ought to be identical to the macroexpanded code, but regardless I will leave it out so we don't have to wonder

gfredericks commented 9 years ago

Okay I have some code here that seems to work except for some bug in the generate-seq code that I don't have time to chase down at the moment. Hopefully the non-seq code is correct and adequate for perf tests.

gfredericks commented 9 years ago

(bug indicated by the one test failure)

dakrone commented 9 years ago

Okay I have some code here that seems to work

Great, thanks! No guarantees for when I will be able to look as my wife just had a baby and I am short on sleep/free-time, but I will try to take a look whenever I can! :)

gfredericks commented 9 years ago

babies babies babies babies babies babies babies babies babies babies babies babies!

dakrone commented 9 years ago

@gfredericks I finally had a second to check out your branch. Performance-wise it looks good to me, would you be able to open a PR for moving to it?

gfredericks commented 9 years ago

Will do

gfredericks commented 9 years ago

In terms of an API for extending via opts, the code used in https://github.com/greglook/puget/issues/23 will probably be relevant.

ikitommi commented 8 years ago

Any news on this? Looking also forward to this, config similar like the edn / transit readers?