Make ElasticSearch.json_encoder private or something #42

Closed
erikrose opened this Issue Dec 12, 2012 · 4 comments

Comments

Projects
None yet
1 participant
Contributor

erikrose commented Dec 12, 2012

I'm a little flummoxed at what the extensibility story is supposed to be for JSON encoding. It looks like there are 2 equivalent hook points:

  • Subclassing ElasticSearch and overriding from_python
  • Sticking an alternative encoder class into ElasticSearch.json_encoder

Have 1.

Contributor

erikrose commented Dec 12, 2012

We need to get this figured out by 1.0 so we don't start exposing a public API we'd later have to retract.

Contributor

erikrose commented Dec 16, 2012

@jezdez I'm curious: why does django-haystack call from_python on everything as it builds the query, rather than letting (for instance) search() do the encoding implicitly? Is it just to fail sooner and provide an easier debugging experience?

Contributor

erikrose commented Dec 16, 2012

@jezdez If you get a chance, I could use your help on erikrose/pyelasticsearch@307271b.

Specifically, it looks like _encode_json and from_python are similar in spirit—they both purport to encode Python to JSON—and yet subtly different in that from_python doesn't quote its strings or change True to "true". (I've left failing tests in my patch unchanged to illustrate this.) Is there a strangeness in haystack we should fix, as per my earlier comment?

More broadly, I'm not sure what to_python and from_python are for, and I'd like to see them go away. It seems to me that any JSON coming back from from ES should be converted to Python types to the fullest extent possible: if to_python knows tricks _decode_response doesn't, we should improve _decode_response. Haystack shouldn't have to call the conversion stuff explicitly at all.

Of course, it's entirely possible I misunderstand something about haystack, not having read the whole thing. I await your response. :-)

Contributor

erikrose commented Mar 18, 2013

Hey @jezdez, are you at PyCon? We've got a pretty good sprint going here. I'd really like to remove those two methods and would value your weighing in.

@ghost ghost assigned erikrose Mar 18, 2013

@erikrose erikrose closed this in c89ad05 Mar 20, 2013

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