Improve Nominatim Provider #806
Conversation
Add following fields: - Attribution - Class - Display name - OSM type - OSM Id - Type
|
Thank you. After a first review I think it looks good. Could you fix the style errors and add some tests? |
Doing it right now :) |
|
Style errors are fixed. I'm quite new with PHPUnit so it might take some time to write the tests ; I'll do my best :) |
|
We just need some integration testing. Ie, write a test and verify the result. The HTTP response will be cached, so make sure to commit the cached file. It is really just copy paste from existing tests. I want a test that shows the result has a class, display name, attribution etc. |
|
Okay, I'll try to figure this out :P |
Add testGeocodeWithRealAddress() ; Check new fields
Forgot to check displayName
|
I think it's done :) |
Fix reverse geocoding
src/Provider/Nominatim/Nominatim.php
Outdated
| @@ -169,7 +174,23 @@ private function xmlResultToArray(\DOMElement $resultNode, \DOMElement $addressN | |||
| $builder->setBounds($bounds['south'], $bounds['west'], $bounds['north'], $bounds['east']); | |||
| } | |||
|
|
|||
| return $builder->build(); | |||
| if (false === $reverse) { | |||
There was a problem hiding this comment.
Do we really need this separation? It seams like most the lines are the same.
I suggest to remove the $reverse parameter.
There was a problem hiding this comment.
Yes, I don't really like that solution with reverse parameter either but the API answer from geocoding or reverse geocoding is quite different so the extraction of the files is also different :
Geocoding:
$location = $location->withDisplayName($resultNode->getAttribute('display_name'));
Reverse geocoding:
$location = $location->withDisplayName($resultNode->nodeValue);
Any suggestion to improve/simplify that ?
There was a problem hiding this comment.
I see.. Lets improve it by only putting the differences in the if-else-statement. It is a private function so we may improve it later if we come up with anything.
There was a problem hiding this comment.
👍 I'll take care of that today !
| @@ -160,6 +164,7 @@ private function xmlResultToArray(\DOMElement $resultNode, \DOMElement $addressN | |||
| $countryCode = strtoupper($countryCode); | |||
| } | |||
| $builder->setCountryCode($countryCode); | |||
|
|
|||
There was a problem hiding this comment.
Same as previous comment, seemd to make more sense to create a "block" with country code process ;
But again, really don't mind putting that back as it was :)
There was a problem hiding this comment.
Okey. I just saw a weird diff. This is fine
| @@ -149,7 +154,6 @@ private function xmlResultToArray(\DOMElement $resultNode, \DOMElement $addressN | |||
| } | |||
| } | |||
|
|
|||
| $builder->setPostalCode($postalCode); | |||
There was a problem hiding this comment.
Seemed more logical to regroup postal code process ;
But I really don't mind putting it back to line 152 ^_^
src/Provider/Nominatim/Nominatim.php
Outdated
| @@ -123,7 +127,7 @@ public function reverseQuery(ReverseQuery $query): Collection | |||
| * | |||
| * @return Location | |||
| */ | |||
| private function xmlResultToArray(\DOMElement $resultNode, \DOMElement $addressNode): Location | |||
| private function xmlResultToArray(\DOMElement $resultNode, \DOMElement $addressNode, string $attribution = 'Data © OpenStreetMap contributors, ODbL 1.0. http://www.openstreetmap.org/copyright', bool $reverse): Location | |||
There was a problem hiding this comment.
Will the default value ever have an effect here?
Let's remove the default value.
| } | ||
|
|
||
| /** | ||
| * @param null|string $osmId |
|
Happy new year ! 🎉 |
|
Done ! :) |
Add following fields:
Close #805