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

Add support for custom serializers #50

Merged
merged 4 commits into from Apr 7, 2015

Conversation

3 participants
@msiemens
Owner

msiemens commented Mar 17, 2015

As discussed in #48, it would be nice to have support for custom serialization of objects unknown to TinyDB (= not supported by the JSON backend). This implements this interface, but I'm unsure how useful it will be.

cc @eugene-eeo @EmilStenstrom

Add support for custom serializers
See #48 for rationale
@EmilStenstrom

This comment has been minimized.

Show comment
Hide comment
@EmilStenstrom

EmilStenstrom Mar 17, 2015

This looks excellent to me. Will use for all projects where I want to store dates in the database.

EmilStenstrom commented Mar 17, 2015

This looks excellent to me. Will use for all projects where I want to store dates in the database.

@eugene-eeo

This comment has been minimized.

Show comment
Hide comment
@eugene-eeo

eugene-eeo Mar 17, 2015

Contributor

I remain skeptical but I think there must be a better way of doing this... Currently the code looks quite complex and no offence, "hack-ish" but I'm sure you'd clean it up. Coming from a performance point of view, I think the registered serializers should be stored in a hash-table, i.e.

self._serializers = {}

Then prefix lookup will be very efficient and straightforward. And it also forces developers to choose a unique name ;)

UPDATE: Also better to move the serializing and deserializing parts into a separate class, then add hooks in order to preserve current performance since not every use case demands custom serialization.

Contributor

eugene-eeo commented Mar 17, 2015

I remain skeptical but I think there must be a better way of doing this... Currently the code looks quite complex and no offence, "hack-ish" but I'm sure you'd clean it up. Coming from a performance point of view, I think the registered serializers should be stored in a hash-table, i.e.

self._serializers = {}

Then prefix lookup will be very efficient and straightforward. And it also forces developers to choose a unique name ;)

UPDATE: Also better to move the serializing and deserializing parts into a separate class, then add hooks in order to preserve current performance since not every use case demands custom serialization.

@msiemens

This comment has been minimized.

Show comment
Hide comment
@msiemens

msiemens Apr 7, 2015

Owner

First, sorry for not moving forward with this PR for so long! I hope we're now ready to go.

@eugene-eeo I've now refactored the serialization/deserialization into methods. Now I also use a dict for storing the serializers.

Also better to move the serializing and deserializing parts into a separate class, then add hooks in order to preserve current performance since not every use case demands custom serialization.

I don't think creating a hooking infracstructure will perform better than the current code because when no serializers are registered as the outer for-loop of the (de)serialization will not run at all.

But on the other hand I'm currently toying with the idea of using a middleware for serialization. This could be used like this:

serializer = Serializer(JSONStorage)
serializer.register(DateTimeSerializer(), 'TinyDate')
db = TinyDB(storage=serializer)

# ...

The advantage of this approach is that it doesn't modify the core at all and is totally opt-in. What do you think, @EmilStenstrom, @eugene-eeo?

Owner

msiemens commented Apr 7, 2015

First, sorry for not moving forward with this PR for so long! I hope we're now ready to go.

@eugene-eeo I've now refactored the serialization/deserialization into methods. Now I also use a dict for storing the serializers.

Also better to move the serializing and deserializing parts into a separate class, then add hooks in order to preserve current performance since not every use case demands custom serialization.

I don't think creating a hooking infracstructure will perform better than the current code because when no serializers are registered as the outer for-loop of the (de)serialization will not run at all.

But on the other hand I'm currently toying with the idea of using a middleware for serialization. This could be used like this:

serializer = Serializer(JSONStorage)
serializer.register(DateTimeSerializer(), 'TinyDate')
db = TinyDB(storage=serializer)

# ...

The advantage of this approach is that it doesn't modify the core at all and is totally opt-in. What do you think, @EmilStenstrom, @eugene-eeo?

@eugene-eeo

This comment has been minimized.

Show comment
Hide comment
@eugene-eeo

eugene-eeo Apr 7, 2015

Contributor

👍 for not modifying core. By separating hooking infrastructure I meant the same thing as keeping it out of core and attaching the hooking class as a middleware, sorry for not explaining myself clearly.

Contributor

eugene-eeo commented Apr 7, 2015

👍 for not modifying core. By separating hooking infrastructure I meant the same thing as keeping it out of core and attaching the hooking class as a middleware, sorry for not explaining myself clearly.

@msiemens

This comment has been minimized.

Show comment
Hide comment
@msiemens

msiemens Apr 7, 2015

Owner

@eugene-eeo I've updated this PR again, now with a Middleware that does the serialization. The only downside is that its usage is now less ergonomic (see docs/extend.rst).

Owner

msiemens commented Apr 7, 2015

@eugene-eeo I've updated this PR again, now with a Middleware that does the serialization. The only downside is that its usage is now less ergonomic (see docs/extend.rst).

@eugene-eeo

This comment has been minimized.

Show comment
Hide comment
@eugene-eeo

eugene-eeo Apr 7, 2015

Contributor

@msiemens Just a suggestion, I think I must be frustrating you but why not make it an extension? Custom serialisation is not something that we'd expect to be in core.

Contributor

eugene-eeo commented Apr 7, 2015

@msiemens Just a suggestion, I think I must be frustrating you but why not make it an extension? Custom serialisation is not something that we'd expect to be in core.

@EmilStenstrom

This comment has been minimized.

Show comment
Hide comment
@EmilStenstrom

EmilStenstrom Apr 7, 2015

As I've said before, I'm in favor of having this in core. Having dates in JSON is very common in my experience, and being able to point to the serialization docs is a big advantage over having to install an extension in my opinion.

EmilStenstrom commented Apr 7, 2015

As I've said before, I'm in favor of having this in core. Having dates in JSON is very common in my experience, and being able to point to the serialization docs is a big advantage over having to install an extension in my opinion.

@msiemens

This comment has been minimized.

Show comment
Hide comment
@msiemens

msiemens Apr 7, 2015

Owner

I agree with @EmilStenstrom about the extension question. I think it's okay to ship the middleware with tinydb as it's totally opt-in and we already provide a middleware that could be moved to an extension (CachingMiddleware).

Owner

msiemens commented Apr 7, 2015

I agree with @EmilStenstrom about the extension question. I think it's okay to ship the middleware with tinydb as it's totally opt-in and we already provide a middleware that could be moved to an extension (CachingMiddleware).

msiemens added a commit that referenced this pull request Apr 7, 2015

@msiemens msiemens merged commit 1dcb1df into master Apr 7, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@msiemens msiemens deleted the custom-serialization branch Apr 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment