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

Closed
wants to merge 2 commits into from

2 participants

@balsdorf

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.

@balsdorf balsdorf Don't explicitly check if S() type is None, instead try to call metho…
…d on the specified type and handle NotImplementedError.
fd2878b
@willkg
Mozilla member

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

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
Mozilla member

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

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.

@balsdorf balsdorf closed this Mar 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment