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.

Sign up for GitHub

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

Jump to bottom

Add composite Aggregation #1804

Merged
merged 2 commits into from
Oct 22, 2020
Merged

Add composite Aggregation #1804

merged 2 commits into from
Oct 22, 2020

Conversation

itkg-ppottie
Copy link
Contributor

#1774 with TU & TF +changelog

@itkg-ppottie itkg-ppottie mentioned this pull request Oct 7, 2020
Closed
@deguif
Copy link
Collaborator

deguif commented Oct 7, 2020

@itkg-ppottie Could you add this new aggregration to the aggregation DSL: Elastica\QueryBuilder\DSL\Aggregation

deguif
deguif reviewed Oct 7, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
deguif
deguif reviewed Oct 7, 2020
View reviewed changes
tests/Aggregation/CompositeTest.php Outdated Show resolved Hide resolved
tests/Aggregation/CompositeTest.php Outdated Show resolved Hide resolved
tests/Aggregation/CompositeTest.php Outdated Show resolved Hide resolved
tests/Aggregation/CompositeTest.php Outdated Show resolved Hide resolved
@itkg-ppottie itkg-ppottie force-pushed the add-composite branch 2 times, most recently from 589a3d2 to ad22274 Compare October 7, 2020 10:34
@itkg-ppottie itkg-ppottie force-pushed the add-composite branch 2 times, most recently from d0c629c to f97f10e Compare October 7, 2020 10:38
deguif
deguif reviewed Oct 7, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@itkg-ppottie itkg-ppottie force-pushed the add-composite branch 2 times, most recently from 8ca0aba to a0b6755 Compare October 7, 2020 10:50
ruflin
ruflin reviewed Oct 12, 2020
View reviewed changes
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. Should I directly merge or you want to update the test method name?

src/Aggregation/Composite.php Show resolved Hide resolved
tests/Aggregation/CompositeTest.php
/**
* @group functional
*/
public function testCompositeNoAfterAggregation(): void
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the functional test.

tests/Aggregation/CompositeTest.php Show resolved Hide resolved
@ruflin
Copy link
Owner

ruflin commented Oct 21, 2020

@itkg-ppottie There seems to be also a merge conflict here now. Could you rebase?

@Ciloe Ciloe mentioned this pull request Oct 21, 2020
Closed
@deguif
Copy link
Collaborator

deguif commented Oct 22, 2020

@itkg-ppottie do you have time to finish this one? Would be nice to get it merged ;)

@itkg-ppottie itkg-ppottie force-pushed the add-composite branch 3 times, most recently from b59dced to d5eba0d Compare October 22, 2020 10:18
@deguif deguif merged commit 35dbcc0 into ruflin:master Oct 22, 2020
@deguif
Copy link
Collaborator

deguif commented Oct 22, 2020

Thanks @itkg-ppottie

@deguif deguif mentioned this pull request Oct 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruflin ruflin ruflin left review comments

@deguif deguif deguif left review comments

Assignees
No one assigned
Labels
hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@itkg-ppottie @deguif @ruflin