search

geocoder-php / Geocoder

The most featured Geocoder library written in PHP.
https://geocoder-php.org
MIT License
3.94k stars 515 forks source link

Here provider: additionalData field. #956

Closed mariusasensi closed 4 years ago

mariusasensi commented 5 years ago

Here Provider: Special field "additionalData" in provider address model and fill adminLevels of Address base model with the data from that field.

(https://developer.here.com/documentation/geocoder/topics/resource-type-response-geocode.html#resource-type-response-geocode__address)

jbelien commented 5 years ago

Thanks @mariusasensi ! Could you make sure your PR passes the Travis CI build ? Thanks :)

mariusasensi commented 5 years ago

Thanks @mariusasensi ! Could you make sure your PR passes the Travis CI build ? Thanks :)

I understand that I must add the response of the test request response in ".cached_response" just like you commented in this thread, right?

mariusasensi commented 5 years ago

Thanks @mariusasensi ! Could you make sure your PR passes the Travis CI build ? Thanks :)

I understand that I must add the response of the test request response in ".cached_response" just like you commented in this thread, right?

I answer myself 😄 Yes, I needed to upload the response to my request to pass the checks, I just update my PR.

jbelien commented 5 years ago

Looks good, thanks a lot 👍

Any reason you limited the PR to ['CountryName', 'StateName', 'CountyName'] ?

Quickly reading the documentation, I notice that additional data can also be used for the query, could you extend the PR to implement this ?

Thanks !

mariusasensi commented 5 years ago

Looks good, thanks a lot 👍

Any reason you limited the PR to ['CountryName', 'StateName', 'CountyName'] ?

I limited to those levels because I see that in other providers the adminLevels field collects less specific address fields. But, I think it's a good idea not to limit the adminLevels of the address and to eliminate the specific field that I have added in the specific address.

Quickly reading the documentation, I notice that additional data can also be used for the query, could you extend the PR to implement this ?

Thanks !

Sure! In a few days I try to expand the provider. 💪

jbelien commented 5 years ago

@mariusasensi Here provider as been updated with #955 ; don't forget to pull that update ! :)

mariusasensi commented 5 years ago

I just updated the pr adding the possibility to add the filters: country, state, county and city in the query of Here (with their tests).

jbelien commented 5 years ago

Hello @mariusasensi ,

I'm a bit confused (but I'm probably missing something here) !

I see that you use "country", "state", "county", and "city" as "AdditionalData" but none of the actual AdditionalData field described here (both for Request and Response).

And for the "Response", I think it's to just parse $location['Address']['AdditionalData'] because it can contain much more than admin levels (CrossingStreet0, PostalCodeType, ...).

mariusasensi commented 5 years ago

Hello @mariusasensi ,

I'm a bit confused (but I'm probably missing something here) !

I see that you use "country", "state", "county", and "city" as "AdditionalData" but none of the actual AdditionalData field described here (both for Request and Response).

And for the "Response", I think it's to just parse $location['Address']['AdditionalData'] because it can contain much more than admin levels (CrossingStreet0, PostalCodeType, ...).

Okay, in the next few days I will add the chance to use the AdditionalData filters in the query and answer parser.

mariusasensi commented 5 years ago

Good night!

Sorry for forgetting the PR, I've been pretty busy with my work.

I see that you use "country", "state", "county", and "city" as "AdditionalData" but none of the actual AdditionalData field described here (both for Request and Response).

As you told me, I've added all the filters to get extra data in the request (except one*), as indicated in the Here documentation. Each extra filter is added using withData indicating its key/value. For the request, additionaldata=key,value;key2,value2. The filter I didn't add is IncludeDistanceMarkers because the documentation says "The option is supported for the retrieveAddresses mode in the reversegeocode endpoint."

And for the "Response", I think it's to just parse $location['Address']['AdditionalData'] because it can contain much more than admin levels (CrossingStreet0, PostalCodeType, ...).

Done! Finally, I have changed how it gets the name of the country and I have also added another field to get Shape if you use the IncludeShapeLevel filter.

jbelien commented 4 years ago

Hello @mariusasensi ,

Sorry I didn't take care of your PR sooner, July was bit busy.

We just released a new version of the Here provider (0.2.1), could you make sure your changes do not conflict with this new version ? If not, I'll merge this PR :)

mariusasensi commented 4 years ago

Hello @mariusasensi ,

Sorry I didn't take care of your PR sooner, July was bit busy.

We just released a new version of the Here provider (0.2.1), could you make sure your changes do not conflict with this new version ? If not, I'll merge this PR :)

Good night! I have updated my PR with Master. I think it won't make conflicts, but it would be more comfortable if you merge it with Master and then, in case of conflict with the new version of the provider, I fix it. Thank you very much! @jbelien