Added the geo_centroid aggregation #1150

Merged
merged 5 commits into from Jul 27, 2016

Conversation

Projects
None yet
2 participants
@coreation
Contributor

coreation commented Jul 25, 2016

No description provided.

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Jul 26, 2016

Owner

LGTM. Would be nice to have at least one integration test added to confirm it works as expected. Best is having a look at the existing integration tests and then do mostly copy / paste :-)

Owner

ruflin commented Jul 26, 2016

LGTM. Would be nice to have at least one integration test added to confirm it works as expected. Best is having a look at the existing integration tests and then do mostly copy / paste :-)

@coreation

This comment has been minimized.

Show comment
Hide comment
@coreation

coreation Jul 26, 2016

Contributor

@ruflin Added integration test and added the class to the Changelog. Not sure how I can see the merge conflict though.

Contributor

coreation commented Jul 26, 2016

@ruflin Added integration test and added the class to the Changelog. Not sure how I can see the merge conflict though.

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Jul 27, 2016

Owner

Best is to rebase on top of master. Probably it is a conflict in CHANGELOG.md

Owner

ruflin commented Jul 27, 2016

Best is to rebase on top of master. Probably it is a conflict in CHANGELOG.md

CHANGELOG.md
@@ -9,7 +9,7 @@ All notable changes to this project will be documented in this file based on the
### Bugfixes
### Added
-
+- `Elastica\Aggregations\GeoCentroid`

This comment has been minimized.

@ruflin

ruflin Jul 27, 2016

Owner

Could you add a newline afterwards?

@ruflin

ruflin Jul 27, 2016

Owner

Could you add a newline afterwards?

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Jul 27, 2016

Owner

Changes LGTM, only minor comment.

Owner

ruflin commented Jul 27, 2016

Changes LGTM, only minor comment.

@coreation

This comment has been minimized.

Show comment
Hide comment
@coreation

coreation Jul 27, 2016

Contributor

Apparently I couldn't find a way to use rebase, my commits were on the master branch of my fork. Not that experienced with rebasing, so I did a merge upstream from the origin master, that indeed showed that the changelog had some differences before I changed it. Looks good now I think :)

Contributor

coreation commented Jul 27, 2016

Apparently I couldn't find a way to use rebase, my commits were on the master branch of my fork. Not that experienced with rebasing, so I did a merge upstream from the origin master, that indeed showed that the changelog had some differences before I changed it. Looks good now I think :)

CHANGELOG.md
@@ -38,6 +39,7 @@ All notable changes to this project will be documented in this file based on the
- Fix php notice on `\Elastica\Index::getAliases()` if index has no aliases [#1078](https://github.com/ruflin/Elastica/issues/1078)
### Added
+- `Elastica\Aggregations\GeoCentroid`

This comment has been minimized.

@ruflin

ruflin Jul 27, 2016

Owner

Seems like your merge now caused it to be added twice. This one should be removed.

@ruflin

ruflin Jul 27, 2016

Owner

Seems like your merge now caused it to be added twice. This one should be removed.

CHANGELOG.md
@@ -10,6 +10,7 @@ All notable changes to this project will be documented in this file based on the
### Added
- Elastica\QueryBuilder\DSL\Query::geo_distance
+- Elastica\Aggregation\GeoCentroid

This comment has been minimized.

@ruflin

ruflin Jul 27, 2016

Owner

If you already edit the changelog, you could also add #1150 at the end (see change examples below)

@ruflin

ruflin Jul 27, 2016

Owner

If you already edit the changelog, you could also add #1150 at the end (see change examples below)

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Jul 27, 2016

Owner

Yeah, if you work in master directly rebasing on master is hard ;-) Merging cause a small issue above.

Owner

ruflin commented Jul 27, 2016

Yeah, if you work in master directly rebasing on master is hard ;-) Merging cause a small issue above.

@coreation

This comment has been minimized.

Show comment
Hide comment
@coreation

coreation Jul 27, 2016

Contributor

You were right, I added it to the wrong version. Should be ok now I think.

Contributor

coreation commented Jul 27, 2016

You were right, I added it to the wrong version. Should be ok now I think.

@ruflin ruflin merged commit 62fafe0 into ruflin:master Jul 27, 2016

2 checks passed

codecov/project 85.20% (+0.01%) compared to a25b6c7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Jul 27, 2016

Owner

Thanks for the contribution. I squashed to commits and put it into master.

Owner

ruflin commented Jul 27, 2016

Thanks for the contribution. I squashed to commits and put it into master.

@coreation

This comment has been minimized.

Show comment
Hide comment
@coreation

coreation Jul 27, 2016

Contributor

You're very welcome, loving the library so I'm glad I can give back (small as it may be ;) )

Edit: Any idea on when a new release is coming out, because now to enjoy the GeoCentroid I'll have to put my version number to dev-master afaik.

Contributor

coreation commented Jul 27, 2016

You're very welcome, loving the library so I'm glad I can give back (small as it may be ;) )

Edit: Any idea on when a new release is coming out, because now to enjoy the GeoCentroid I'll have to put my version number to dev-master afaik.

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Jul 27, 2016

Owner

No exact plans for the next as there are already a few changes in an no BC breaks I could do a release quite soonish.

One thing I wanted to fix is the httpproxy vulnerability before the next release: https://httpoxy.org/ Unfortunately Elastica is also affected here (in case you use HHVM_VERSION) and Guzzle Transport:

$proxy = getenv('http_proxy') ?: null;

Owner

ruflin commented Jul 27, 2016

No exact plans for the next as there are already a few changes in an no BC breaks I could do a release quite soonish.

One thing I wanted to fix is the httpproxy vulnerability before the next release: https://httpoxy.org/ Unfortunately Elastica is also affected here (in case you use HHVM_VERSION) and Guzzle Transport:

$proxy = getenv('http_proxy') ?: null;

@coreation

This comment has been minimized.

Show comment
Hide comment
@coreation

coreation Jul 27, 2016

Contributor

Ok, thanks for the info, I'm afraid I have too little time to dive into that one :)

Contributor

coreation commented Jul 27, 2016

Ok, thanks for the info, I'm afraid I have too little time to dive into that one :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment