greshake / i3status-rust

Very resourcefriendly and feature-rich replacement for i3status, written in pure Rust
GNU General Public License v3.0
2.86k stars 472 forks source link

Use lat/lon for Open Weather Map #1948

Closed bim9262 closed 1 year ago

bim9262 commented 1 year ago

Fixes #1485

MaxVerevkin commented 1 year ago

Do you think it makes sense to remove useless spaces/newlines in open_weather_map_cities.json like met_no_legends.json?

bim9262 commented 1 year ago

If there's ever an update to the files it would be very hard to read the diff (as a sanity check) if it's all on a single line. Since the line breaks are not a factor binary size when using bincode I'd leave the line breaks there. Also where did you find the met_no_legends? I can add it to the update script.

bim9262 commented 1 year ago

I'm considering restructuring the json file to keep the keep the id as a number so that we can pack it smaller by not having strings as keys (need to do some testing though)

MaxVerevkin commented 1 year ago

If there's ever an update to the files it would be very hard to read the diff (as a sanity check) if it's all on a single line

Good point.

Also where did you find the met_no_legends?

Here: https://web.archive.org/web/20230328155110/https://api.met.no/weatherapi/weathericon/2.0/legends.

MaxVerevkin commented 1 year ago

to keep the keep the id as a number

Yeah, I just wanted to ask why you converted them to strings, as it doesn't work is some cases. For example the last entry of the file is

  "7.12969E+7": {
    "lon": 33.92,
    "lat": 44.59
  }
MaxVerevkin commented 1 year ago

Is it necessary to prefetch this info? In case with met.no we had to because they disabled the API. The 1M binary increase is unfortunate.

bim9262 commented 1 year ago

There's two errors like that, but they come from the source file. I opened a support case with OWM...

Based on the errors in the other city list they publish they accidentally made them floats (those same IDs ended with .0 in that format)

bim9262 commented 1 year ago

Is it necessary to prefetch this info? In case with met.no we had to because they disabled the API. The 1M binary increase is unfortunate.

We could fetch it on demand. We'd probably want to give the lowest precedence to city_id since it would become the most expensive to load.

bim9262 commented 1 year ago

This is now with live fetching. It caches the result of the file to disk and compares the last modified date on the server with the local modification date. (owm does not respect the If-Modified-Since header).

Switched over to the larger city list (as described at https://openweathermap.org/bulk#list): List of major cities across the globe - 22,635 cities. You can download the full list of cities here Extended cities list across the globe - 209,579 cities. You can download the full list of cities here

bim9262 commented 1 year ago

After all that I got this response from OWM's support:

You can still use city IDs for your API requests, we will not remove/disable them. However, we have deprecated the city ID list, meaning we don't update the list or correct issues due to it being mainly open source.

So perhaps we should continue to send the city_id, but query the geo api for place and zip.

MaxVerevkin commented 1 year ago

we don't update the list or correct issues due to it being mainly open source

I can't make sense of this sentence...

So perhaps we should continue to send the city_id, but query the geo api for place and zip.

If they won't actually remove support for this, and it makes our code simpler, then why not.

bim9262 commented 1 year ago

we don't update the list or correct issues due to it being mainly open source

I can't make sense of this sentence...

So perhaps we should continue to send the city_id, but query the geo api for place and zip.

Keep the code I wrote for the place name and zip codes in this pr, but use the city_id like we were doing originally

MaxVerevkin commented 1 year ago

Keep the code I wrote for the place name and zip codes in this pr, but use the city_id like we were doing originally

I was referring to their rationale for not fixing city IDs. "open source" == "unmaintained"? But yes, let's use city_id as we were doing it.

bim9262 commented 1 year ago

Keep the code I wrote for the place name and zip codes in this pr, but use the city_id like we were doing originally

I was referring to their rationale for not fixing city IDs. "open source" == "unmaintained"? But yes, let's use city_id as we were doing it.

I was half asleep when I read your message. Yeah I don't understand "open source" == "unmaintained" either.

This was the response about where the data comes form:

We get our city database from https://www.geonames.org/. However, you can't find the weather IDs there, IDs were generated by us. Since the list has been deprecated and was not updated for a while some city IDs could be missed or point to other coordinates.

We strongly recommend using geographical coordinates instead of city IDs and using Geocoder API: https://openweathermap.org/api/geocoding-api