Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modifying the FilterOp structure #177

Closed
wants to merge 24 commits into from
Closed

Conversation

mikeyoon
Copy link
Contributor

I'm more interested in starting a discussion on the FilterOp architecture. In my fork, I'm currently going with setting up FilterOp as a monolithic struct that can hold everything that fits into the FilterDsl. It seems easier to me to manage to me, but I'm curious to see what others think. It still has a ways to go before supporting the entirety of FilterDsl.

Mike Yoon and others added 8 commits March 21, 2015 23:02
Modified QueryDsl to add a query parent node to filtered queries when
serializing
* Added an AddRange function for creating a range filter
* Change QueryDsl to return a query root node when there's no filter
@mattbaird
Copy link
Owner

Hi @mikeyoon good stuff! Thanks for contributing. Personally, I think I like the approach you are pursuing. If it came with even more tests, that would be wonderful.

@mikeyoon
Copy link
Contributor Author

Great, I'll flesh it out a bit more and add tests soon. Thanks.

@mikeyoon
Copy link
Contributor Author

mikeyoon commented Apr 9, 2015

I've fleshed out the pull request and added tests for the FilterOp. The tests just check the structure of the resulting JSON rather than querying the mock data, since I assume the queries should work if they're constructed according to the Elastic Search spec.

I've made some breaking changes, mostly affecting anyone who uses FilterOps directly. Don't know the best way to proceed, but I'm not terribly vested in anything. I've updated the Vagrant file to use apt-get instead of chef, but not sure how many people were using that instead of the Docker option.

There is one thing I would like to be better, but I currently don't have a great understanding of Go pointers and haven't had time to research more. On the compound filter types (And, Or, Not), I'd prefer the functions I made take in a ... slice instead of one filter at a time.

@otiai10
Copy link

otiai10 commented Apr 30, 2015

Looks so good! I'm desiring it to be merged.
Thank you @mikeyoon @mattbaird

@mattbaird
Copy link
Owner

Hey guys - I'm going to need to pull/test/review and then tag a pre-release since (i believe) this is a breaking change. I'm swamped at the moment, so this could take a bit. My apologies in advance.

@mikeyoon
Copy link
Contributor Author

Closing so I can base this pull request off of a branch. I want my fork's master to represent the version of elastigo that I need to use in my projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants