Clarified usage of custom queries, added exclude, made internal methods private.

[dblock]

I went down a rabbit hole starting with an untested exclude method in Estella::Query that wasn't used anywhere.

First, I made it useful by supporting exclude in estella_search.

Then, we document that you can override query_definition. But we also have a bunch of methods like add_sort that we don't document but do override in gravity. I propose we kill those by making them private in favor of customizing queries as any Ruby object, in the initializer, as documented in this change.

This can be a major version bump, and I can do the gravity change as well.

[coveralls]

Coverage increased (+0.6%) to 99.441% when pulling 60c8445 on custom-query into 5639fe1 on master.

[coveralls]

Coverage increased (+0.6%) to 99.441% when pulling 60c8445 on custom-query into 5639fe1 on master.

[cavvia]

add_filters is still a more accurate name for this method - it adds filters to the query, not just any indexed fields. the filters are sourced from the list of indexed_fields but we are not simply adding indexed fields to the query, filters is a more precise term.

[cavvia]

this is great - you're quite right, exclude was an undocumented helper, so i think full integration into estella_search is a good idea.

i'm not sure add_query or field_factors should be private methods, as clients may want to override these too. we mainly override add_query in gravity when customizing our queries because it provides greater control than just overriding the query_definition. although you can technically override private methods in a sub-class, i'm not convinced it's a good pattern. i'd rather they be part of a public interface if they are methods we expect clients to override in their child class.

of course, you might want to only go down the 'replace the initializer' route for all customization, but i think just overriding the core query body without worrying about aggregations, sorts, etc is a common use case.

[dblock]

I've renamed the method back to add_fields and made field_factors public and added a note on it in README.

Generally I feel pretty strongly not to customize methods like add_*, I really think that that logic should move to the initializer, and that's what I was planning to do for Gravity. Mostly because this is an unnecessarily complicated interface - the order of these add methods doesn't even matter and they seem to be introduced for logical separation rather than for creating a true "interface".

Plus, when you override these methods you're potentially breaking what you intended to implement in searchable, especially if you forget to call super.

LMK how you feel about this?

[coveralls]

Coverage increased (+0.6%) to 99.441% when pulling 350fd63 on custom-query into 5639fe1 on master.

[cavvia]

I agree on not customizing add_* methods if possible - that was just a pattern i inherited with this code. I'm happy for us to override the initializer, but it might still be good to have a query body hook of some sort.

It's true that the order of the add_ methods doesn't matter here.

BTW I no longer seem to be an owner of the gem for some reason, can you add anil@artsy as an owner? You're the only owner now. I wanted to push v0.2.2.

[dblock]

Isn't query_definition the query body hook?

Added you as owner, sorry about that. Are you going to push 0.2.2 before merging this change? Tell me what you want me to do in here :)

[cavvia]

query_definition is the documented hook but that still uses a query template defined in add_query, so if you want to customize the entire query body you have to override that, which we do in Gravity.

This PR looks good to me now - going to merge it in.

I pushed v0.2.2 to rubygems in the meantime as that had your critical fix for document deletion.

[cavvia]

I've pushed v0.3.0 now too.

[dblock]

Thanks @cavvia, I'll work on getting 0.3.0 into Gravity.