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

Don't explicitly check if S() type is None #202

Closed balsdorf closed 10 years ago

balsdorf commented 10 years ago

I ran into a problem trying to write a MappingType but still use the behavior of an untyped S() such as searching all indexes. This patch removes special handling for untyped S() and allows other MappingTypes to access the same behavior.

I couldn't find any discussion about this behavior so I'm not sure if this was intentional but I think this change does not break backwards compatibility and allows more flexibility in the future. Please let me know if there is anything else I can do on this.

willkg commented 10 years ago

I'm not sure I understand the use case. If you have a type, why are you using an untyped S? Why not instead either have your type return the list of indexes to use or extend S to pick the right indexes?

balsdorf commented 10 years ago

I'm not using an untyped S, I have a type that I want to use all available indexes and doctypes instead of returning an explicit list. This is impossible to do, even if I return None from my methods since return values are wrapped in a list. My usage of elasticutils is to search a number of indexes created by a different system.

My other way of dealing with this was to extend S but this was a simpler and better way IMO. I am new to elasticsearch and elasticutils though so perhaps my approach is not the best.

willkg commented 10 years ago

I still don't understand the use case here, nor do I understand why this is simpler. Can you write a test that illustrates the problem?

Also, it looks like you're adding support for match_phrase_prefix along with this PR. That should be a separate PR accompanied with a test case. Generally, it's best to do feature work in a separate branch and then make the PR against that branch rather than do it against your master branch.

balsdorf commented 10 years ago

Ugh, I'm new to using pull requests with github and thought it would only merge the specific commit. I'm going to close this pull request and will re-submit a separate branch when I write a clear cut test.

Thanks for your work on this, elasticutils has been a life saver.