Google component filters #466
Conversation
.gitignore
Outdated
There was a problem hiding this comment.
This should go to your global gitignore.
|
@geocoder-php/geocoder WDYT? |
…lters # Conflicts: # tests/Geocoder/Tests/Provider/GoogleMapsTest.php
|
Useful addition! 👍 |
|
It's a good addition - the component filters improve really the result :) 👍 |
|
@mvcaaa could you rebase this PR please? |
|
@willdurand Done |
in/sync w/source
|
Any feedback ? |
|
I know it has been a while since this PR was submitted. But I can't find the documentation of component filtering. The link you provided does not work... (or Im blind) |
|
They changed it at some point to https://developers.google.com/maps/documentation/geocoding/intro#ComponentFiltering |
Nyholm
left a comment
Nyholm
left a comment
There was a problem hiding this comment.
Great. I like this PR. But I think we test the wrong things. We could only mock the message factory and validate the URL and we would be good.
|
|
||
| /** | ||
| * @param string $query | ||
| * @return \Geocoder\Model\AddressCollection |
There was a problem hiding this comment.
Remove this. It is not a part of the PR and it is no longer valid.
| $provider->geocode('10 avenue Gambetta, Paris, France'); | ||
| } | ||
|
|
||
| public function testGeocodeWithValidComponentFilters() |
There was a problem hiding this comment.
Would this test be different without setComponentFilters?
I suggest that you only write a unit test that make sure the correct parameters is added on the URL. We already have functional tests that make sure the response handing is proper.
There was a problem hiding this comment.
ComponentFilters are 100% optional - so without it this test will not be different, but it will fail 100%
Not sure how i can redo this - any suggestions are welcome.
There was a problem hiding this comment.
Instead of doing this functional tests we could do unit tests that verifies that the query URL has changed when using setComponentFilters. We do not actually need to execute the query and verify the result.
There was a problem hiding this comment.
Yes, you are 100% right, but
Here is my concern: query is formed by private function buildQuery(and only there). I can test it, but it isn the perfect solution, kind of, i think
There was a problem hiding this comment.
What if you mock the HttpClient and verify that the sendRequest function has a request with the preferred query?
There was a problem hiding this comment.
It can be done, sure. Will do if no one was willing to disagree or suggest something better.
I think, buildQuery must be refactored also - but i will not do that. It is way outside my small tiny PR.
There was a problem hiding this comment.
I think, buildQuery must be refactored also - but i will not do that. It is way outside my small tiny PR.
Yes it is outside the scope.
I do not think you have to refactor anything. Just update the test code.
There was a problem hiding this comment.
@mvcaaa i did something silimar for googles result_type & location_type and wrote a unittest that checks the url.
Check out here: https://github.com/arubacao/Geocoder/blob/8f322d551b486c8c3bf6fd7fe84caaec6364797e/tests/Geocoder/Tests/Provider/GoogleMapsTest.php#L304
| } | ||
|
|
||
| /** | ||
| * Add components for filtering. |
There was a problem hiding this comment.
Really good that you provide the link, but it should be updated.
| } | ||
|
|
||
| /** | ||
| * @param $region |
There was a problem hiding this comment.
This doc block is not part of the PR. You can keep this but try to avoid "code cleanups" in the same PR as features.
|
Ping, will you complete the work on this PR or should it be closed? |
|
Im closing because inactivity. Feel free to create a new PR. I like this feature. |
8 participants
Fully based on pull/427
Tests added
Some fixes to original code
Closes #426