GNOME Bugzilla – Bug 538787
Extend API to return non-string values and machine parsable forecast info
Last modified: 2008-11-27 05:57:52 UTC
I would like to use libgweather in Evolution, but it's kinda unusable for that at the moment, because every value is returned as localized string, but I would rather use the actual data, so I will not parse data from strings. The second thing is forecast information, I would prefer to use the machine parseable strings, rather than natural language description of the forecast. As you told me, the NOAA is capable of such information, and as you suggested, some way to tell the library what kind of forecast information I prefer would help here. So also the API change here.
Created attachment 112979 [details] [review] proposed lgw patch for libgweather; I thought about something like this. I do not know how much it is acceptable for you, I can change it, if you wish. The thing about forecast, I found, just by chance, an XML equivalent for the forecast data for US, maybe we can use that, but they use different lat/lon, so this doesn't work now. I have no idea how to convert between them. The other thing, we can probably use this XML format, but it will be definitely better to have there some ours, independent on the source, so in case other country will produce its own format, then we will convert to our. I skipped this part until that XML will be accepted and working. (I didn't add ChangeLog entry yet.)
Wow, you're fast :-) I don't have time to really review the patch (Dan might do it), but one thing that looks wrong is: gboolean weather_info_get_temperature_raw (WeatherInfo *info, const gchar *kind, TempUnit unit, gdouble *value) kind shouldn't be a string but should be an enum. If we want to break API, I'd also name your function weather_info_get_temperature() and rename weather_info_get_temperature() to weather_info_get_temperature_for_display() or something like this. Not sure it's worth breaking the API, though...
(In reply to comment #1) > The thing about forecast, I found, just by chance, an XML equivalent for the > forecast data for US, maybe we can use that, but they use different lat/lon, so > this doesn't work now. I have no idea how to convert between them. WeatherLocation's latitude and longitude fields are in radians, because that's what you need for distance and sunrise/sunset calculations. RADIANS_TO_DEGREES() in weather-priv.h will fix them for you. > we can probably use this XML format, but it will be definitely better to > have there some ours, independent on the source, so in case other country will > produce its own format, then we will convert to our. I skipped this part until > that XML will be accepted and working. (I didn't add ChangeLog entry yet.) I don't think there's really much reason to export XML data, rather than parsing the XML in libgweather and then exporting it as an array of structs. bug 533787 talks about using a longitude/latitude-based interface to US forecast data (for a different reason), and I've already started working on that, although I ran into a problem, because if you use the interface mentioned in that bug report, it automatically switches to giving you a marine forecast rather than a terrestrial one if it thinks the lat/lon you gave it point to somewhere in the ocean, and it thinks that the coordinates for Boston that we have in Locations.xml are in the ocean. But the URL you pointed to doesn't have that problem, so I'll update my code to use that. bug 526074 "add numerical functions to get weather info" also overlaps with part of this
*** Bug 520176 has been marked as a duplicate of this bug. ***
(In reply to comment #2) > kind shouldn't be a string but should be an enum. I chose a string intentionally, because of (possible) feature changes, to have break API as small as possible. By adding new temperature value, nothing will change for new and old applications, also new application can use old library. If you do not like the idea, then I can break whole API, just add for each of them a _get_raw_ equivalent. (In reply to comment #3) > I don't think there's really much reason to export XML data, rather than > parsing the XML in libgweather and then exporting it as an array of structs. I agree, I do not know why I didn't think of that before, this is much better than parsing again and again. > bug 533787 talks about using a longitude/latitude-based interface to US > forecast data .... for Boston that we have in Locations.xml are in the ocean. > But the URL you pointed to doesn't have that problem, so I'll update my code > to use that. If you work on that, then I will not touch that part. Just tell me what to do with the API and I'll resubmit the patch tomorrow.
*** Bug 526074 has been marked as a duplicate of this bug. ***
(In reply to comment #5) > If you work on that, then I will not touch that part. I might not get to it in the next few days, so if you're eager to hack on it now, I'll attach my code there. > Just tell me what to do with the API and I'll resubmit the patch tomorrow. 1) I committed a major code style cleanup patch to try and get rid of the inconsistency in libgweather. I think if you just fix the function declarations to put a newline between the return type and the function name you'll be good. (The other big changes were to consistently use 4-space indents and spaces between function names and their open paren, but it looks like you already do that.) 2) Having the functions return the value as their return value and "gboolean *ok" as an out parameter is inconsistent with most of the rest of GNOME, which would have a gboolean return value and an out parameter for the "real" return value. 3) I think I'm fine with you breaking API if you want, if you make patches for the weather and clock applets too. If you don't, using "get_foo_value" like the bug 526074 patch rather than "get_foo_raw" might be nicer? 4) Also from the 526074 patch, WeatherInfo already has temperature_unit, speed_unit, pressure_unit, and distance_unit fields. Any reason you can't use those rather than requiring units to be passed in?
(In reply to comment #5) > (In reply to comment #2) > > kind shouldn't be a string but should be an enum. > > I chose a string intentionally, because of (possible) feature changes, to have > break API as small as possible. By adding new temperature value, nothing will > change for new and old applications, also new application can use old library. Adding a new value to an enum won't break API either. FWIW, I think it's better to get a nice API now (since this library is not widely used yet) even if it means breaking the API, than wondering how to fix the API in a few years :-)
(In reply to comment #7) > (In reply to comment #5) > > If you work on that, then I will not touch that part. > > I might not get to it in the next few days, so if you're eager to hack on it > now, I'll attach my code there. If it's just few days, then no hurry, really. > 1) I committed a major code style cleanup patch to try and get rid of the > inconsistency in libgweather. I think if you just fix the function declarations > to put a newline between the return type and the function name you'll be good. > (The other big changes were to consistently use 4-space indents and spaces > between function names and their open paren, but it looks like you already do > that.) Sure, maybe if you commit to trunk before me, then I can make it consistent. With spaces, I think my editor places somewhere an 8-spaced TAB, instead of spaces, somehow. > 2) Having the functions return the value as their return value and "gboolean > *ok" as an out parameter is inconsistent with most of the rest of GNOME, which > would have a gboolean return value and an out parameter for the "real" return > value. These are only local (static) functions, but sure, can be changed too. > 3) I think I'm fine with you breaking API if you want, if you make patches for > the weather and clock applets too. If you don't, using "get_foo_value" > like the bug 526074 patch rather than "get_foo_raw" might be nicer? I woke up this morning and I got the idea about just two functions: gboolean weather_info_get_num_value (*info, enum name, gdouble *value, gint unit); gboolean weather_info_get_time_value (*info, enum name, time_t *value); The unit will not be used for all value names. Also I thought the name as string would be better because it's used as that for properties in glib/gtk and so on, but I'm fine with enums too. Also for 'sky' and other enumerated values, the enum will be cast to double and caller will recast it back. There will be probably another function to get a forecast list of objects too, but that's part of the above thing. > 4) Also from the 526074 patch, WeatherInfo already has temperature_unit, > speed_unit, pressure_unit, and distance_unit fields. Any reason you can't use > those rather than requiring units to be passed in? You know, when using this library, you can specify exact units only when creating the weather info, after that you have no access to change it. So I decided to have a param for that. I didn't do that yet, but I will add a possibility to enter there a ..._DEFAULT value, and in that case it will use a one stored in WeatherInfo structure. -------------------------------------------------------- I'm not sure, maybe I look on this in too complicated way. What do you think?
(In reply to comment #9) > Sure, maybe if you commit to trunk before me, then I can make it consistent. It's already committed > With spaces, I think my editor places somewhere an 8-spaced TAB, instead of > spaces, somehow. Oh, yeah, that's fine (that's how it already is). It's just that 4 spaces is the "base" indentation. > > 2) Having the functions return the value as their return value and "gboolean > > *ok" as an out parameter is inconsistent with most of the rest of GNOME, > These are only local (static) functions, but sure, can be changed too. Oh, I missed that. Do whatever's more convenient for you then. > I woke up this morning and I got the idea about just two functions: > gboolean weather_info_get_num_value (*info, enum name, gdouble *value, gint > unit); > gboolean weather_info_get_time_value (*info, enum name, time_t *value); meh. i like the separate accessors better. > > 4) Also from the 526074 patch, WeatherInfo already has temperature_unit, > > speed_unit, pressure_unit, and distance_unit fields. Any reason you can't use > > those rather than requiring units to be passed in? > > You know, when using this library, you can specify exact units only when > creating the weather info, after that you have no access to change it. So I > decided to have a param for that. I didn't do that yet, but I will add a > possibility to enter there a ..._DEFAULT value, and in that case it will use a > one stored in WeatherInfo structure. OK... I haven't actually *used* the weather API at all, I've just been working with the locations end of things, so I don't have a strong opinion or sense of right/wrong with this. So do what makes sense. (Other maintainers may have stronger opinions, and should probably say so quickly if so :)
(In reply to comment #10) > OK... I haven't actually *used* the weather API at all, I've just been working > with the locations end of things, so I don't have a strong opinion or sense of > right/wrong with this. So do what makes sense. (Other maintainers may have > stronger opinions, and should probably say so quickly if so :) FWIW, I don't have any strong opinion right now. I'll likely have one in the future but just ignore me ;-)
Created attachment 113113 [details] [review] proposed lgw patch ][ for libgweather; Only extending the API. Also prepared for the forecast list, but it doesn't work yet, I do not have a mood to play with "parsing" the XML at the moment. You'll be quicker with that hopefully. I changed my mind (one more time, I'm sorry), I wrote equivalents for all weather_info_get_... as weather_info_get_value_..., no joining similar things together. The patch is now created against trunk (the previous was against gnome-2-22 branch), so should be fine.
vuntz, Dan, can something happen here? Explosions, or fireworks, or even a commit? Or do we want to handle this more safely and commit after branching libgweather for 2.25? Sometimes libraries do have to be extended for the application that want to use them, ain't? ;-)
The patch is incomplete; it doesn't actually parse the machine-parsable forecast. There are other parts of it that *could* be committed, but I was assuming they're not useful to evolution without the forecast-parsing part?
Yes, that's right, it's not complete, but it extends API, so I hoped we can put it inside and then just do the ugly work when the time permits. Sounds good?
Created attachment 123184 [details] [review] proposed lgw patch ]I[ for libgweater; Huh, I'm sorry it took so long, but here we go finally.
This all basically looks good. We still haven't branched libgweather for 2-24, but that should probably happen after 2.24.2 goes out. Also, just a warning, WeatherInfo will probably go away and be replaced by a nicer and more-language-bindable "GWeatherInfo" at some point (as with WeatherLocation -> GWeatherLocation), and likewise all of the enum types eventually need proper namespacing and such. But all of the work you've done will be able to be reused for that. I haven't looked at the XML request/parsing in detail, and reserve the right to come back and harass you later if I think you're grabbing the wrong fields out of the forecast. :)
Sure, feel free to come back to me :) I do not have commit rights for libgweather, please commit for me when the time comes. Also feel free to do anything you want with the code to match libgweather needs.
(In reply to comment #18) > I do not have commit rights for libgweather svn.gnome.org doesn't have separate acls for separate modules. But I've committed the patch for you. Thanks for the code, and if/when I do get around to rewriting it I'll file an evolution bug pointing out the exciting new API. :)
Committed revision for future reference http://svn.gnome.org/viewvc/libgweather?view=revision&revision=507