Revise search raw

[tilgovi]

@Treora your thoughts, please. I removed the extra commit I caused you to make (sorry, again!) and then I spotted a place where the Document tests were using the wrong signature for search(). Finally, it seemed like kwargs were being passed around in places where they were discarded or unused and params seemed unnecessary if we just use kwargs for that role.

Let me know what you think.

[Treora]

I'm surprised I missed that wrong signature for search(). Even more scrutiny next time.

About the kwargs versus params, my choice had to do with allowing easy inheritance. Before my changes there was no query argument, query terms were passed via **kwargs, which was a bad idea as keyword arguments and query terms got mixed in the same dict and could shadow each other (e.g. a field called 'limit' would have been unsearchable). I thought that in general it may be good to keep things with separate purposes separate. The Elasticsearch search parameters are a better candidate for passing via **kwargs, because the arguments ES's search accepts are known. Or at least, they could be known: a reason for keeping the parameters in params is that when adding keyword arguments one then need not know ES's search's argument list. No mixing up, no shadowing. But it's not an important reason.

I had a stronger reason for having some methods accept (and pass on) **kwargs, even if unused in those methods. It makes it easier to add keyword arguments when overriding a method. For example, build_query in Annotation listened for the keyword argument user, and calling search(user='bob') would 'gate' the argument to build_query. For this to work, search had to always pass all **kwargs it receives to build_query, and the original definition of build_query thus has to allow any **kwargs, even though it doesn't use them.
With this PR build_query does not need the user argument anymore (well spotted, I forgot to remove it), so the need for passing **kwargs around disappeared. However, I found this pattern to be helpful to easily make changes to signatures in inherited classes, so maybe it's ok to keep. I even started to think it would be nice if this 'gating' of arguments to overridden methods would somehow be implicit in the language, but maybe I'm just coming up with clumsy solutions for problems that should be solved in other ways.

In both these issues, I'm fine with either solution. I'm still trying to find out elegant programming patterns that create nice abstractions, allow for easy code reuse and inheritance.

[tilgovi]

I'm okay with requiring subclasses to implement search() with an additional keyword if they want to pass that to a different _build_query, especially since the latter is private.

I understand what you mean about not wanting to have to know what arguments do and don't conflict with Elasticsearch, but I actually lean toward thinking it may be a good thing. It forces a problem when there would otherwise just be an ambiguity. I don't want a situation where the same key as a keyword argument and as a param do different things. This way, we're forced to name our keyword arguments in ways that don't collide with ES features.

Is that a reasonable line of thinking?

[Treora]

I'm okay with requiring subclasses to implement search() with an additional keyword

In this project I'm totally okay with it too, as it's all simple. I think that in general the gating pattern is a bit nicer. Your suggestion implies one would have to duplicate the search function's code (and duplication is evil), and upstream changes to search are missed out on.

Background story is that I more often find that I want to make a slight modification to a library's functions, but end up having to clone large portions of code. Having the freedom to replace subfunctions provides a partial solution to this. It does make that the subfunction (_build_query here) should be considered part of the API, which limits the freedom to change code without breaking the API. Any tips about good programming patterns for this are welcome. I'm especially wondering what could be done in a language's design to improve code reusability, for example Python's classmethod seems helpful already.

This way, we're forced to name our keyword arguments in ways that don't collide with ES features.

An issue is that we're not really forced to: one has to manually check the ES API (and watch it for changes), as there will be no warnings on a keyword collision. I'm thinking our language may be to primitive, having keyword arguments in a flat list rather than using some structured approach. Just like variables have namespaces so they don't shadow each other.

Anyway, for practical matters, any of our approaches are fine. This conversation is mostly philosophical. I'll let you choose which branch to merge (my branch is also rebased and updated).

[tilgovi]

I'll give @nickstenning a chance to look.

[nickstenning]

Sorry folks -- I'll review this tomorrow. Shattered today.

[tilgovi]

No problem, @nickstenning. Just to clarify, it's between this and #91 with the difference being that this PR removes some kwargs where they weren't used which revealed to me that there were places they were unused that an explicit params argument was used instead. So I killed params in favor of kwargs.

Either PR or a combination of the two (kill kwargs where not needed but still use params) is fine with me.

[nickstenning]

This looks good to me. Honestly, I think I do prefer the use of an explicit parameter rather than keyword arguments to pass params through to the elasticsearch API. It seems less likely to blow up in our faces later on.

But, that's something we can do later. This pull request is clear improvement as it is.

[Treora]

But, that's something we can do later.

Later? Now is always a good moment. #102