msiemens / tinydb

TinyDB is a lightweight document oriented database optimized for your happiness :)
https://tinydb.readthedocs.org
MIT License
6.83k stars 549 forks source link

Reconsider ujson Support? #306

Closed rogerhurwitz closed 4 years ago

rogerhurwitz commented 4 years ago

First, I'd like to thank everyone for the great work you have done in developing and maintaining tinydb. It is a fantastic right-sized database for a wide range of use cases, and the extensible architecture is an accomplishment in its own right.

Now on to the issue, I noticed that ujson support was deprecated (#263) which made sense at the time given the state of the ujson project. I noticed recently, however, that ujson appears to be once again maintained and apparently active. Given the significant performance advantage, would it make sense to revisit the question of ujson support?

Thanks, Roger

msiemens commented 4 years ago

Hey Roger,

I've thought about this and I think that even if ujson support itself may be useful, I don't think we should include it in the TinyDB core. I want the TinyDB core to be as minimal as possible and it shouldn't be too difficult to implement a storage just like JSONStorage that uses ujson. If someone wants to implement something like tinydb-ujson I'd love to include it in the extensions list.

msiemens commented 4 years ago

Also thank you for your kind words 🙂

msiemens commented 4 years ago

To provide a bit more background: initial ujson support somewhat broken from the beginning. If a user had ujson installed in their environment, there was no way for them to use JSONStorage with Python's regular json. In addition, there are some differences in handling edge-cases with ujson compared to Python's json which may confuse users if ujson is used automatically.

The best way to implement ujson support is creating a new UJsonStorage which doesn't need to be implemented in TinyDB core. That way it is clear which JSON runtime is used if ujson is present in the user's environment.

rogerhurwitz commented 4 years ago

Hi Markus,

Thanks for the thoughtful reply, and I understand the rationale for keeping ujson out of the TinyDB core - makes sense.

Roger