open-weather-ruby-client icon indicating copy to clipboard operation
open-weather-ruby-client copied to clipboard

Support location names in other than ISO 3166

Open mattlindsey opened this issue 11 months ago • 8 comments

Not a huge deal, but data = client.current_weather(city: city, units: units) seems to blow up with Faraday::ResourceNotFoundError error when given given Atlanta, GA, imperial, but works fine with Atlanta, Georgia, imperial. Data issue?

mattlindsey avatar Mar 09 '24 19:03 mattlindsey

What’s the stack? Error from the server?

dblock avatar Mar 10 '24 15:03 dblock

Below is the Issue with the error message and context. Could be I'm doing something wrong, and this is not urgent. I'm not sure exactly how to see the server responses.

I created a hacky fix around the exception, but would be nice to not need it.

https://github.com/andreibondarev/langchainrb/pull/193

Here's where I initially saw the exception: https://github.com/andreibondarev/langchainrb/issues/175

mattlindsey avatar Mar 10 '24 16:03 mattlindsey

image

Possibly it is supposed to work this way? If it can't find the city it raises exception?

mattlindsey avatar Mar 10 '24 19:03 mattlindsey

City, state and country are passed through as is here into the OpenWeather API documented in https://openweathermap.org/current#name.

It says: City name, state code and country code divided by comma, Please refer to ISO 3166 for the state codes or country codes. You can specify the parameter not only in English. In this case, the API response should be returned in the same language as the language of requested location name if the location is in our predefined list of more than 200,000 locations.

So the API uses https://www.iso.org/obp/ui/#iso:code:3166:US, which says it's Georgia (and not 'GA') for the state of. You'll have to use something like https://github.com/carmen-ruby/carmen (maybe there exists a newer library? let us know if you find one that works?) to normalize/convert these.

I'd welcome documentation on the fact that the city/state is ISO 3166 and guidance to convert those, so will leave this issue open. Could possibly be a feature, but at least documentation. Maybe you care to help?

dblock avatar Mar 13 '24 00:03 dblock

Thanks for explanation! I can probably also tell the LLM in the Langchainrb gem to always return the full state name according to ISO when 'function calling' my Weather tool. And I will also submit PR soon here to update your doc concerning ISO 3166.

mattlindsey avatar Mar 13 '24 13:03 mattlindsey

When I test different scenarios, the behavior seems a little strange, so you or I can expand the README at line 89 to the following if you want:

Returns weather by city, optional state (in the US) and optional ISO 3166 country code.

client.current_city('Sydney')                    # Good
client.current_city('London, UK')                # Good
client.current_city('London', 'UK')              # Good
client.current_city('Albany')                    # Good
client.current_city('Albany, New York')          # Good
client.current_city('Albany, New York', 'US')    # Good
client.current_city('Albany, NY', 'US')          # Good
client.current_city('Albany', 'New York', 'US')  # Good
client.current_city('Albany', 'NY', 'US')        # Good
client.current_city('Albany', 'NY')  # Bad: 2 letter state abbreviation without country will give Faraday::Resource not found
client.current_weather(city: 'Albany', state: 'NY', country: 'US') # Good

mattlindsey avatar Mar 14 '24 12:03 mattlindsey

Adding examples is good. I would not add the "# Good" comment because they are all good and not say anything about the one that doesn't work. You could say "The OpenWeather API requires locations in the ISO 3166 format. Names that cannot be resolved will cause the API call to fail with Faraday::ResourceNotFoundError ." or something like that.

dblock avatar Mar 14 '24 19:03 dblock

But in the case of state, ISO 3166 is not required, since NY and MA (for example) do (usually) work. And I think we should keep the Bad line since it documents a case that common sense says should work.

Update: I submitted a PR and will take out the 'Bad' line if you want.

mattlindsey avatar Mar 14 '24 19:03 mattlindsey