Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support configurable serialization #75

Merged
merged 8 commits into from
Jun 7, 2016
Merged

Conversation

crisptrutski
Copy link
Owner

@crisptrutski crisptrutski commented Jun 5, 2016

Closes #73

Sorry for the delay @dotboris, we just missed each other last time I passed through.

Just before implementing this, I realised that using the optional xform parameter (eg. (mr/sync-r ref (partial into (sorted-map)))) should also be sufficient. But that does involve a awkward extra allocation path over the data, and doesn't handle nested maps. Handling those would make it even more awkward 😆

Here I've just made sorted-maps the default. But the implementation leaks out, and the clojure.lang.Sorted property comes at a price: keys are now typed.

((into (sorted-map) {:a 1 :b 2}) :a)
 => 1

((into (sorted-map) {:a 1 :b 2}) "a")
 ;; Java.lang.ClassCastException: java.lang.String cannot be cast to clojure.lang.Keyword

(into (sorted-map) {:a 1 'c 2}) => 
 ;; Java.lang.ClassCastException: java.lang.String cannot be cast to clojure.lang.Symbol

This could break quite a bit of old code, so I'm not completely comfortable with this. Perhaps using something like Linked's order-preserving maps would be preferable, with the added bonus of then working with queries and/or custom priorities.

But I'm loath to add another dependency, and also this "preserve insert order" property is perhaps more confusing than helpful when you start manipulating the data. Or perhaps not a real issue - since the default keys are server generated and monotonically increasing..

The cop out would be to just implement #51 and let poor users choose their trade-off.

That burns down the issue queue fastest, so I'm copping out 😄

@crisptrutski
Copy link
Owner Author

crisptrutski commented Jun 5, 2016

Have implemented a rough version, you can try it out with:

(matchbox.serialization.plain/set-default!)
(matchbox.serialization.keyword/set-default!) ;; default
(matchbox.serialization.sorted/set-default!)
(matchbox.serialization.stable/set-default!)

The last option requires linked library to be added manually as a project dependency.

Before merging I would want to speed up the dispatch of m/serialize and m/hydrate. To be cross platform and support advanced compilation, I'm guessing that means using some native mutables.

@crisptrutski
Copy link
Owner Author

Now also closes #51

@crisptrutski crisptrutski changed the title Return sorted maps by default Support configurable serialization, with batteries included Jun 5, 2016
@crisptrutski crisptrutski changed the title Support configurable serialization, with batteries included Support configurable serialization Jun 5, 2016
@crisptrutski
Copy link
Owner Author

Overhead is probably dealt with sufficiently now. Feels a bit overwrought though, will sleep on it all.

@crisptrutski crisptrutski merged commit 8c40fd1 into master Jun 7, 2016
@crisptrutski crisptrutski deleted the sorted-by-default branch June 7, 2016 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant