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 distance precision to GeohashGrid.php #1884

Merged
merged 4 commits into from
Dec 7, 2020

Conversation

krewetka
Copy link
Contributor

@krewetka krewetka commented Dec 2, 2020

As described in https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-geohashgrid-aggregation.html#_options_3 there is possibility to pass precision as string.

It was also working in earlier versions of Elastica.

Got broken after adding int to parameter in setPrecision.

I think it is better to add second method for that.

@ruflin
Copy link
Owner

ruflin commented Dec 3, 2020

@krewetka Could you also rebase this PR now that CI changes are in?

@ruflin ruflin requested a review from deguif December 3, 2020 09:56
@krewetka krewetka force-pushed the precision-in-km branch 2 times, most recently from bd59693 to bc1b4af Compare December 4, 2020 13:04
@krewetka
Copy link
Contributor Author

krewetka commented Dec 4, 2020

@krewetka Could you also rebase this PR now that CI changes are in?

just so you know CI is failing sometimes for some strange reason. I had a fail, changed just single test name a bit , restarted it and next time it was ok

image

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.

Change LGTM, but waiting on green. Also @deguif should have a final look. Please merge directly if it looks good to you.

@deguif deguif merged commit a5c87f1 into ruflin:master Dec 7, 2020
@deguif
Copy link
Collaborator

deguif commented Dec 7, 2020

Thanks @krewetka

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.

3 participants