dblock / open-weather-ruby-client

OpenWeather Ruby Client
MIT License
33 stars 19 forks source link

Support location names in other than ISO 3166 #35

Open mattlindsey opened 6 months ago

mattlindsey commented 6 months ago

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?

dblock commented 6 months ago

What’s the stack? Error from the server?

mattlindsey commented 6 months ago

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 commented 6 months ago

image

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

dblock commented 6 months ago

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?

mattlindsey commented 6 months ago

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 commented 6 months ago

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
dblock commented 6 months ago

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.

mattlindsey commented 6 months ago

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.