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

Added few Span queries #1320

Merged
merged 52 commits into from
Jul 25, 2017
Merged

Added few Span queries #1320

merged 52 commits into from
Jul 25, 2017

Conversation

mhernik
Copy link
Contributor

@mhernik mhernik commented Jun 2, 2017

I added 4 span queries:

  • SpanMulti
  • SpanNear
  • SpanOr
  • SpanTerm

@ruflin
Copy link
Owner

ruflin commented Jun 2, 2017

Wow, you not only added 1 span query but all of them. Something seems to fail with the tests.

@ruflin
Copy link
Owner

ruflin commented Jun 2, 2017

@alekitto @mhernik Could it be that the two of you are working on the same thing at the same time? #1319

@mhernik
Copy link
Contributor Author

mhernik commented Jun 3, 2017

Hi,
Yeah, I was doing some last minute changes on machine without docker, so without running tests. I'll bo back to it on Monday.
@alekitto Seems like it. What a coincidence, a 4 year old issue, and 2 PRs for it on same day :)

@alekitto
Copy link
Contributor

alekitto commented Jun 3, 2017

@mhernik Yeah, what a coincidence! I think we can merge our work, as my PR is still missing span_near and span_or queries, as some functional tests you've already written. What do you think about it?

Marek Hernik added 2 commits June 5, 2017 18:31
@mhernik
Copy link
Contributor Author

mhernik commented Jun 5, 2017

Hi,
I've updated PR to pass the tests.
@alekitto We definitely should merge our work! What do you propose? Do you want to make a PR to my fork?

@ruflin
Copy link
Owner

ruflin commented Jun 6, 2017

+1 on merging the work. PR to @mhernik branch sounds like a good idea to me.

@ruflin
Copy link
Owner

ruflin commented Jun 13, 2017

@mhernik @alekitto Please ping me when you think this is ready for review.

@mhernik mhernik changed the title Added few Span queries [WIP] Added few Span queries Jun 13, 2017
@mhernik
Copy link
Contributor Author

mhernik commented Jun 13, 2017

@alekitto is further working on span_within and span_not queries. Will ping you, when it's ready!

@mhernik mhernik changed the title [WIP] Added few Span queries Added few Span queries Jul 7, 2017
@mhernik
Copy link
Contributor Author

mhernik commented Jul 7, 2017

Hi @ruflin,
Actually I got him @alekitto wrong. He said that he's working on on span_within and span_not queries, but will create additional PR for that. So that PR is ready now for review! :)

Copy link
Owner

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Only a few minor comments.

*
* @return $this
*/
protected function _addQuery($type, $args)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This more looks like _setQuery as in case a second query is set, it will overwrite the previous one.

*/
public function __construct($match = null, $end = null)
{
if (null !== $match) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*
* @return $this
*/
protected function _addQuery($type, $args)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See prev. comment.

@ruflin
Copy link
Owner

ruflin commented Jul 10, 2017

@mhernik Could you check the review above and squash the commits into 1?

ruflin and others added 19 commits July 24, 2017 10:29
* Update elasticsearch-php dependency to 5.2
* Update incompatible test
* Update README page
* Update CHANGELOG
* Update liniting

added filter path option

added filter path option

added changelog entry and asserts to config test
…er volume sharing solved, now snapshot tests are working (ruflin#1298)
Added a new param to Client::request and Request::_construct to allow
setting the content-type of the data being sent.
This defaults to "application/json" and is only overidden by Bulk and MultiSearch
where it should be application/x-ndjson.

Sadly I'm not sure how to test this properly.
I'm not a big a big fan of adding a new param like that. Ideally we should refactor
the $data param of Request::_construct to accept a array (defaults to application/json)
or a new RequestBody object where the client could set the content-type alongside the
the string.

closes ruflin#1301
added changelog entry and asserts to config test

added SpanOrTest

added SpanMulti

span functions in dsl query updated

removed unnecessary attributes

reverted docker ip range

Changelog update

Changelog update
Updated DSL queries
Test implementation of new classes

add span_term, span_multi, span_first query

CR
@mhernik
Copy link
Contributor Author

mhernik commented Jul 24, 2017

Hi @ruflin. Looks like squashing also merged other people's commits into mine PR.
I think it will be better to make a new PR, or is there a way to fix it?

@ruflin
Copy link
Owner

ruflin commented Jul 24, 2017

@mhernik You should be able to rebase. But if that becomes too messy, just open a new PR.

@ruflin
Copy link
Owner

ruflin commented Jul 25, 2017

@mhernik It looks like the PR still only contains your changes, right? So if I squash it it will be only 1 commit afterwards. But you should be able to rebase too I think if you want to have it clean in the PR.

@ruflin ruflin merged commit eee063c into ruflin:master Jul 25, 2017
@ruflin
Copy link
Owner

ruflin commented Jul 25, 2017

@mhernik I merged this as I really wanted to have it in as I plan a release the next hours / days. I hope this works for you.

@ruflin ruflin mentioned this pull request Jul 25, 2017
9 tasks
@mhernik
Copy link
Contributor Author

mhernik commented Jul 25, 2017 via email

@ruflin
Copy link
Owner

ruflin commented Jul 25, 2017

@mhernik Do you know what is going to happen with #1319

@mhernik
Copy link
Contributor Author

mhernik commented Jul 25, 2017 via email

@alekitto
Copy link
Contributor

I'll rebase my work upon master and push the work into #1319 in a day.

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.