etianen / django-watson

Full-text multi-table search application for Django. Easy to install and use, with good performance.
BSD 3-Clause "New" or "Revised" License
1.21k stars 129 forks source link

allow custom json dumping for meta #123

Closed samuelcolvin closed 9 years ago

samuelcolvin commented 9 years ago

currently if you want to customise the behaviour of the json dump method you would have to either:

This is particularly useful for things like lazy translations or Decimals where customer encoders can be very helpful in JSONifying data.

If you think this makes sense, I can also add:

etianen commented 9 years ago

This is an excellent idea, but I'd suggest generalising it a little:

  1. Call the method serialize_meta, which opens the doors to people using a non-JSON encoding, and puts it into the public namespace of the class.
  2. Add another method deserialize_meta, which is a hook for turning it back into a dict again. Use this method instead of json.loads in the SearchEntry body.
  3. As you suggested, use the Django JSON encoder for the serialize_meta implementation. This takes care of datetimes, I believe, which is nice.

I'm not sure that tests are needed, as the current test suite already checks that meta can be saved and loaded.

samuelcolvin commented 9 years ago

will do.

On a related note, I don't think meta is documented anywhere. Maybe worth adding to the wiki.

samuelcolvin commented 9 years ago

Ok, sorry for delay, I think this is done. Django's DjangoJSONEncoder isn't as powerful as one might hope, so I've extended it to cope with a few more types, code partially borrowed from raven-python.

etianen commented 9 years ago

This looks pretty sweet.

One last question: do you think that the serialize/deserialize meta methods belong on SearchAdapter or SearchEngine? Overriding them on SearchAdapter would seem to be easier for the majority of users, since overriding SearchEngine is a bit more arcane.

samuelcolvin commented 9 years ago

yes, i thought that when i was making the change but didn't want to change any more than I had to.

I'll move both methods to the adapter.

etianen commented 9 years ago

Wonderful! Thanks for your patience in getting this just right. :)