lugensa / scorched

Sunburnt offspring solr client
MIT License
27 stars 19 forks source link

Issue 41 highlighting #42

Closed mlissner closed 7 years ago

mlissner commented 7 years ago

This should fix issue #41 by mirroring the solr_highlighting attribute that was available in Sunburnt. I had to make a small tweak to the from_json method so that it would know the correct unique key, but otherwise I think this is a fairly straightforward fix.

Also includes tests, and I bumped the version and change log, in hopes of making your job easier.

Please let me know what you think.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.07%) to 95.32% when pulling 3d5b0bec0289a18b05f7c89443b3434b9e7accae on mlissner:issue-41-highlighting into aa3be453f19b87f6cb7864b80d0d2d32f5a5b3a0 on lugensa:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.03%) to 95.42% when pulling 2f18eecf9113c95d7460991aa0fd1abb1212a6e1 on mlissner:issue-41-highlighting into aa3be453f19b87f6cb7864b80d0d2d32f5a5b3a0 on lugensa:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.1%) to 95.273% when pulling afae7565ff023b55ac59545528d729aef34a262b on mlissner:issue-41-highlighting into aa3be453f19b87f6cb7864b80d0d2d32f5a5b3a0 on lugensa:master.

mlissner commented 7 years ago

This ended up being a slightly bigger PR than I had hoped for. I just pushed some more commits that do a couple of additional things:

  1. I discovered a bug in #38 that made it so that multi-value dates were not returned properly. Fixed.
  2. The grouping was significantly under developed, so this fleshes it out significantly, including adding solr_highlights attributes to the groups.

The big picture here is that users can use highlighting or groups or both or neither, and the code needs to support all of that.

It should be a lot better now, with support for all of the above. I've also added a lot of tests, so I look forward to your review and hopefully a new release as soon as possible.

Thank you!

delijati commented 7 years ago

@lujh any objections? Looks good to me!

lujh commented 7 years ago

I just merged the PR. @delijati I would raise the version to 0.11.0 ok?

mlissner commented 7 years ago

@lujh, it looks like you got a 👍 from @delijati on the bump to 0.11.0. Do you know when you'll be able to release that?