Feature/azure maps #1667
Feature/azure maps #1667
Conversation
AhlOct
commented
Nov 11, 2024
•
AhlOct
commented
- Lookups
- Results
- Tests
- Documentation
|
Thanks for this. It looks great. Just one thing: there's no need to add the :limit option in configuration.rb. It should work fine without that. If you can remove that, I believe it will fix the erroring test. Thanks! |
There was a problem hiding this comment.
Sorry, there were a few additional issues. In addition to what I've mentioned in comments:
- You need to add a result fixture called
azure_madison_square_garden. This is expected by the tests that run on all lookups to make sure global result attributes are supported. - After making all the recommended changes, you'll still see one failing test. The failure message is "Lookup azure does not return empty array when no results." Can you look into fixing that?
Thanks. And let me know if you have any questions about any comments.
lib/geocoder/lookups/azure.rb
Outdated
| params = { | ||
| 'api-version' => 1.0, | ||
| 'language' => query.options[:language] || 'en', | ||
| 'limit' => configuration.limit || 10, |
There was a problem hiding this comment.
Here we need to use configuration[:limit] instead of configuration.limit, since :limit is not a global config option. (See my comment in test/unit/lookups/azure_test.rb for more info.)
test/unit/lookups/azure_test.rb
Outdated
|
|
||
| def setup | ||
| super | ||
| Geocoder.configure(lookup: :azure, limit: 1) |
There was a problem hiding this comment.
Since :limit is Azure-specific (not a global config option), it needs to be set in a separate hash: Geocoder.configure(lookup: :azure, azure: {limit: 1})
lib/geocoder/results/azure.rb
Outdated
| end | ||
|
|
||
| def coordinates | ||
| @data['position'] || {} |
There was a problem hiding this comment.
The coordinates method must return a 2-element array with [lat,lon]. So we need something like this:
if @data['position'].is_a?(String) # reverse geocoding result
@data['position'].split(',').map(&:to_f)
elsif @data['position'].is_a?(Hash) # forward geocoding result
[@data['position']['lat'], @data['position']['lon']]
end
test/unit/lookups/azure_test.rb
Outdated
| result = Geocoder.search('Jakarta').first | ||
|
|
||
| assert_equal -6.17476, result&.coordinates['lat'] | ||
| assert_equal 106.82707, result&.coordinates['lon'] |
There was a problem hiding this comment.
Because we've restructured the coordinates return value, this should be:
assert_equal -6.17476, result&.coordinates[0]
assert_equal 106.82707, result&.coordinates[1]
lib/geocoder/lookups/azure.rb
Outdated
| 'subscription-key' => configuration.api_key | ||
| } | ||
|
|
||
| params.merge!(super) |
There was a problem hiding this comment.
For readability, I would remove the ! from the method name, since params is discarded when the method exits (the in-place merging is irrelevant, we're just returning the result of the merge).
lib/geocoder/lookup.rb
Outdated
| @@ -78,6 +79,7 @@ def ip_services | |||
| @ip_services ||= [ | |||
| :baidu_ip, | |||
| :abstract_api, | |||
| :azure, | |||
There was a problem hiding this comment.
Please remove this line. :azure is for physical addresses, not IP addresses.
… not return empty array when no results.
|
Thanks for your suggestions. I've fixed all issues you have mentioned and test 100% passed in my local. Now, I think we can run test again. |
README_API_GUIDE.md
Outdated
| @@ -61,6 +61,19 @@ Global Street Address Lookups | |||
| ``` | |||
| * Via environment variables and other external methods. See **Setting AWS Credentials** in the [AWS SDK for Ruby Developer Guide](https://docs.aws.amazon.com/sdk-for-ruby/v3/developer-guide/setup-config.html). | |||
|
|
|||
| ### Azure (`:azure`) | |||
|
|
|||
| * **API key**: required (set `Geocoder.configure(lookup: :azure, api_key: "your_api_key", limit: your_limit)`) | |||
There was a problem hiding this comment.
The info in parentheses is duplicated below. Let's remove it here.
README_API_GUIDE.md
Outdated
| * **Documentation**: https://learn.microsoft.com/en-us/azure/azure-maps | ||
| * **Terms of Service**: https://azure.microsoft.com/en-us/support/legal | ||
| * **Limitations**: Azure Maps doesn't have any maximum daily limits on the number of requests that can be made, however there are limits to the maximum number of queries per second (QPS) (see https://learn.microsoft.com/en-us/azure/azure-maps/azure-maps-qps-rate-limits) | ||
| * **Notes**: To use Azure, set `Geocoder.configure(lookup: :azure, api_key: "your_api_key", limit: your_limit)` :limit - restrict the maximum amount of returned results, e.g. limit: 10. |
There was a problem hiding this comment.
Two things:
- Let's fix the syntax, showing that :limit has to be specified in an
azure: {...}hash. - This makes it sound like :limit is required, which it isn't,, so let's specify the default (10) and mention that it's optional.
There was a problem hiding this comment.
I've updated README_API_GUIDE.md, sorry I forgot to update it yesterday.
