search

mozilla / elasticutils

[deprecated] A friendly chainable ElasticSearch interface for python
http://elasticutils.rtfd.org
BSD 3-Clause "New" or "Revised" License
243 stars 76 forks source link

PR for issue #191: Other facet information #211

Closed eire1130 closed 10 years ago

eire1130 commented 10 years ago

This PR closes out issue #191

willkg commented 10 years ago

Awesome sauce! I'm on a gonna-get-0.9 kick right now. I'll definitely work through this before 0.9 goes out.

willkg commented 10 years ago

I like the ideas here. This could use some code fixing and it could use some unit tests for the additional behavior.

I'd like to include this in 0.9. Is this something you could finish up in the next week? If not, I can toss it in my queue.

eire1130 commented 10 years ago

@willkg Ok, I made a couple changes.

  1. I removed the __eq__ hack and made the tests to work with FacetResult
  2. I made the iter() change suggested above
  3. I added two tests, one for missing documents and one for other documents.

One note about __eq__, some people ~might~ experience some backwards compatability issues if they were ever doing a dict comparison on the resulting facets. Probably not likely, but worthwhile noting.

From my above comments to your comments:

The __getitem__ is probably needed though to maintain backwards compatibility. Anyone expecting a dictionary previously, FacetResult will still work. This is why I raise a KeyError, so anyone expecting a KeyError will get the right Exception class.

willkg commented 10 years ago

I'm really sorry I hadn't gotten to this earlier. Looks like you did the work in your master branch and then rebased on top of that which kind of makes a mess of the PR.

Rather than hassle you to do it over again, I went through the commits, cherry-picked them into a pristine branch and then did a commit that tweaks some of the code style. That's in pr #224.

I'm going to close this in favor of that one.

eire1130 commented 10 years ago

@willkg Cool man! Yeah, that was me being lazy. I have a FacetResult branch, I just let it get away from me for some reason.