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

Add span queries #1319

Merged
merged 1 commit into from
Jul 31, 2017
Merged

Add span queries #1319

merged 1 commit into from
Jul 31, 2017

Conversation

alekitto
Copy link
Contributor

@alekitto alekitto commented Jun 1, 2017

Fixes #1316

  • span_term
  • span_multi
  • span_first
  • span_near
  • span_or
  • span_containing
  • span_within
  • span_not
  • field_masking_span

@ruflin ruflin mentioned this pull request Jun 2, 2017
@ruflin
Copy link
Owner

ruflin commented Jul 25, 2017

#1320 is now merged. This probably needs rebase now.

@alekitto
Copy link
Contributor Author

I've rebased and squashed my work.
The only thing left is the field_masking_span, but it's barely documented (there's only an example), so i think we could leave this out, as the implementation of the query class would be probably wrong.

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. See my minor comment. This could be applied to all setters in the PR.

For field_masking_span agree that we can tackle that in case someone needs it actually.

*/
public function setLittle(AbstractSpanQuery $little)
{
$this->setParam('little', $little);
Copy link
Owner

Choose a reason for hiding this comment

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

We normal have a return $this->setParam('little', $little); in here so all setters return the object itself. This allows for chaining functions.

@alekitto
Copy link
Contributor Author

Ok, fixed all the setters in SpanContaining, SpanNot, SpanWithin and SpanNear (missed in #1320)

@ruflin
Copy link
Owner

ruflin commented Jul 27, 2017

@alekitto In the diff Builder.php shows up and I'm not sure why?

@ruflin
Copy link
Owner

ruflin commented Jul 27, 2017

BTW: Could you also add an entry to the changelog?

@alekitto
Copy link
Contributor Author

Builder.php is an empty file, I probably deleted it without noticing.
The Changelog already contains a line for Span* queries addition. Do I have to add another one for these three?

@ruflin
Copy link
Owner

ruflin commented Jul 27, 2017

Argh, it seems some things got mixed up in the Changelog :-( The reason an additional entry is needed because just yesterday happened a release. So this goes into the next release.

@ruflin
Copy link
Owner

ruflin commented Jul 27, 2017

I just pushed a change to the CHANGELOG to bring it up-to-date. Make sure to get the most recent one in before updating it.

@alekitto alekitto changed the title [WIP] add span queries Add span queries Jul 27, 2017
@alekitto
Copy link
Contributor Author

👍 Done.

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.

Thanks. Waiting for green.

@ruflin ruflin merged commit 0d10e3e into ruflin:master Jul 31, 2017
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.

2 participants