fix all() for a sliced query

[flisky]

I'm surprised that all doesn't consider the slice.

[willkg]

Looks like you're mixing .execute() with .all() which I don't want to do. .all() is literally all as it says in docstring.

Given that, I'm going to reject this.

[flisky]

It doesn't make sense to me. I'm asking the all SearchResults after the slice, why it gives me the results without the slice? It breaks the convention in django, haystack, and many other library just like rest_framework, because we need to use execute rather than all in elasticutils.

On I see, the only purpose of all is the way discard the slice. Is it right? If so, It's a pity for me as a django guy. And maybe it's easy to misunderstand for other guys even they read the docstring like me: how many to return means total, not the asking.

[willkg]

I'm not really sure how this is confusing:

http://elasticutils.readthedocs.org/en/latest/api.html#elasticutils.S.all

If you can suggest better wording, I'm interested.

[kevinastone]

Just to jump in since I ran into this issue as well. Code that expects a Django queryset (like django-rest-framework) will call .all() on a the query set to retrieve the results and then perform slicing on them for pagination. I'd prefer that a different method name be chosen (like fetch_all()) to not conflict with the Django expectations.

As it says in the django queryset docs:

Returns a copy of the current QuerySet

Outside of this conflict, I've been able to use ElasticUtils queries in lieu of django querysets without impact on django-rest-framework since their API overlaps so consistently.

[flisky]

Sorry, not a native English speaker.

Actually, if I could choose without compatibility, I would kill the current all, and change execute to all, since the current all doesn't do much things, and this would serve the django-related libraries well. The user could explicitly do the current all when they realize how elasticutils treats slices, say results[:]...[:] only cares the last slice. Just my 2 cents. Please ignore this.

@kevinastone, I meet the exactly same issue like you. On the other, I just wonder that whether rest_framework should check the all method on the objects. In fact, __iter__ should be enough.

[willkg]

First off, S is not a queryset. I wouldn't advise passing an S around as if it were a queryset. It's not a requirement I've been supporting.

Second, if you want different functionality, you should just subclass S and override it. It's pretty easy to do and doesn't obstruct you much.

I use .all() in a couple of places since it's the only way to get back all the search results (ES defaults to giving only a chunk of search results). We implemented it a while ago. This is the first time I've heard of anyone having difficulties with it.

For now, I'm going to continue to reject this PR. However, if you feel strongly that's an error, I suggest you write up an issue for it and if it gains a lot of attention, I'll look at changing the API.

[flisky]

OK, got it. Thanks you, @willkg.

[willkg]

I thought about the use case that @kevinastone suggested and decided that S really should be usable in places where a Django QuerySet is. There's a lot of usefulness there.

Ergo, I wrote up issue #200 which covers the problem this PR addresses.