Impliment a better way to handle API options

[ZeevG]

Currently some of the API options (https://developer.forecast.io/docs/v2#options) can be accessed by optional function parameters to load_forecast().

The forecast units is an example of an API option which is supported. However there is a need to properly support additional options see #25 #19. Currently these use cases are being supported by manually constructing a url and using forecastio.manual()

I am hesitant to clutter up load_forecast() with many optional parameters especially since it is likely more are made available by forecast.io in the future.

A possible solution is to pass any optional parameters to load_forecast() as a dictionary where it would work something like this...

options = {
    'units': 'si',
    'lang': 'es',
    'solar': True
}

forecastio.load_forecast(api_key, lat, lng, options)

The dictionary key/value would be transformed to the GET parameter name/value - the forecast.io API options are really just GET parameters.

This change would break backwards compatibility and so should be implemented along with the other changes planned for version 2.0 - see Milestone v2.

[luckydonald]

In my opinion an dict-less solution is much cleaner, especially if you are using an IDE (im my case PyCharm) which will help with auto-completion.

also in my opinion the code itself looks more dirty and cluttered:

forecastio.load_forecast(api_key, lat, lng, {'units': 'si', 'lang': 'es', 'solar': True})

is not as clean as this shorter code:

forecastio.load_forecast(api_key, lat, lng, units='si', lang='es', solar=True)

If you worried about upwards compatibility with the plan to actually stop maintaining the code any longer, then this dict would be a solution. Thus the idea is not bad, merging it would enhance and ease the situation when new versions of the api is released. A way I would prefer would be to have both. Register the parameters. And if there is an (optional) options array just append what is inside.

If I remember correctly you can even make the parameters dynamically and so support future versions. (I remember the blessings package - for example - to completely add functions dynamically)

[luckydonald]

Another option would be in just parsing the **kwargs dict, so it will be dynamically and offer parameters. Also, is there progress?

[ZeevG]

There has been progress! You can checkout https://github.com/ZeevG/python-forecast.io/tree/2.0.0 which is a branch I have been working on.

Currently, it uses the dict method for options although it would be a very easy to change that. It also includes a few changes to bring it up to the v2 milestone. https://github.com/ZeevG/python-forecast.io/milestones/v2

[michaelarnauts]

I would like to use this library in HomeAssistant, but I need to use the lang option like in #25, but it's blocked by the rewrite of 2.0. What's the status of these changes? Is this project still active? There are open PR's for quite some time.

[luckydonald]

The (website) api hasn't changed that much since, but still fear of changes there are blocking this change.

I am hesitant to clutter up load_forecast() with many optional parameters especially since it is likely more are made available by forecast.io in the future. @ZeevG, Dec 5, 2014

That has been 3 years ago by now.

[ZeevG]

I haven't had as much time to work on this as I have had in the past but the project is still alive. I make sure to address critical bugs quickly but I haven't spent much time on new features. Is the work around posted on #25 suitable? If not, post there explaining your situation.

2.0.0 is actually mostly ready. You might even be able to use it by pointing pip to the 2.0.0 branch! From memory I think there is some work needed to remove the async api methods, test the geolocation feature and update the docs. If anyone wants to have a go at getting it a bit closer I'm happy to assist and explain in more depth what needs to be done. In any case I'll see if I can make some more progress on it soon.

[luckydonald]

Doing

import forecastio
url = "https://api.forecast.io/forecast/API_KEY/-31.967819,115.87718?lang=es"
forecast = forecastio.manual(url)

Is almost the same as directly using

import requests
url = "https://api.forecast.io/forecast/API_KEY/-31.967819,115.87718?lang=es"
forecast = requests.get(url).json()

I mean, what is wrong with a

load_forecast_with_custom_args_which_will_be_future_proove(api_key, lat, lng, units='si', lang='es', solar=True, foo="bar")

By using **kwargs in forecastio.api.load_forecast

def load_forecast_with_args(api_key, lat, lng, time=None, lazy=False, **kwargs):
    url = "https://api.darksky.net/forecast/{key}/{lat},{lng}" + ",{time}" if time is not None else ""
    if lazy:
        kwargs["exclude"] = 'minutely,currently,hourly,daily,alerts,flags'
    if time is not None and isinstanceof(time, datetime):
        time = time.replace(microsecond=0).isoformat()    
    url = (url+"?{params}").format(
        key=key, lat=lat, lng=lng, time=time, params=urlencode(kwargs)
    )
     return manual(baseURL, callback=callback)

Edit: Lol this looks good, gonna use that myself. You just need imports:

try:  # python 2
    from urllib import urlencode
except:  # python 3
    from urllib.parse import urlencode
# end try
from datetime import datetime

[luckydonald]

Using urlencode also is better than the '%s' stuff.

[ZeevG]

Fair enough. I mean the whole thing is just a very light weight wrapper.

The reason I prefer the options dictionary approach is that it hides a lot of the complexity of the API from users who only need the most simple interface. This is probably the vast majority of users. It still makes the rarer use cases available when needed. To a novice programmer a method signature of load_forecast(api_key, lat, lng, options=None) is much less daunting than the above.

Still, I don't think there is any reason you can't use the 2.0.0 branch today, even via pip. I think pip install git+https://github.com/ZeevG/[email protected] should do it. If you want to beta test it, and raise any feedback that would be great!

[ZeevG]

And good point about urlencode. I'll keep that in mind.

[luckydonald]

I improved the code a bit, while you were writing, you might not got the changes:

def load_forecast_with_args(api_key, lat, lng, time=None, lazy=False, callback=None, **kwargs):
    url = "https://api.darksky.net/forecast/{key}/{lat},{lng}" + (",{time}" if time is not None else "") + "?{params}"
    if lazy:
        kwargs["exclude"] = 'minutely,currently,hourly,daily,alerts,flags'
    if isinstanceof(time, datetime):
        time = time.replace(microsecond=0).isoformat()    
    url = url.format(
        key=key, lat=lat, lng=lng, time=time, params=urlencode(kwargs)
    )
    return manual(url, callback=callback)

This is pretty much the same as your current 16 lines, but shorter, more flexible, and safer/newer code style Plus it doesn't introduces so many new variables.

Your current code for comparison:

def load_forecast(key, lat, lng, time=None, units="auto", lazy=False, callback=None):
    if time is None:
        url = 'https://api.darksky.net/forecast/%s/%s,%s' \
              '?units=%s' % (key, lat, lng, units,)
    else:
        url_time = time.replace(microsecond=0).isoformat()  # API returns 400 for microseconds
        url = 'https://api.darksky.net/forecast/%s/%s,%s,%s' \
              '?units=%s' % (key, lat, lng, url_time,
              units,)
    if lazy is True:
        baseURL = "%s&exclude=%s" % (url,
                                     'minutely,currently,hourly,'
                                     'daily,alerts,flags')
    else:
        baseURL = url
    return manual(baseURL, callback=callback)

[luckydonald]

And I still think, it is more simple to read.

Tell me if that would work for you. You still get the same stuff, but add a layer of future compatibility, while still removing the need to duplicate that code in every user's script who need to use a different language.

[ZeevG]

Sweet! Open a PR. Since it should be backwards and forwards compatible I'd be happy to approve it.

[luckydonald]

Should I replace the load_forecast function, or create it separately?

[HydrelioxGitHub]

I think you should create it separately to keep the backwards compatibility. Thanks @luckydonald

[luckydonald]

I believe it wouldn't break anything, (still accepts the same parameters) but as you wish :D Edit: Wait, you aren't @ZeevG

[HydrelioxGitHub]

No no, I'm just following the development of this library as a user of Home Assistant project. It's just an advice and can do as you and of course @ZeevG want.

By the way I haven't notice that there is unit test in this repository when I made the comment. So it is not very relevant, if there is any backward compatibility problem, tests should detect it.

[HydrelioxGitHub]

Any news on the issue ?

[ZeevG]

Hey, I have had time to do some work on it. The good news is that everything seems to be working pretty well including geocoding. I've used an API similar to what @luckydonald suggested. There are a couple of things left to do, I'll outline them here incase anyone wants to have a go at them.

• Double check that the recent timezone changes in 1.x are also in the 2.0.0 branch. Update the tests which we are skipping to properly test timezones.
• Update the documentation. Really need to do this one before I release, we get lots of beginners using this library so good docs is a must.

I'll push my changes to 2.0.0. Take a look if you're interested.

[HydrelioxGitHub]

👍 great news

[alexlouden]

Another suggestion - apply the settings on the module somehow, rather than the load_forecast call:

forecastio.lang = 'en'
forecastio.units = 'si'
forecast = forecastio.load_forecast(api_key, lat, lng)

[HydrelioxGitHub]

Any ETA for the 2.0.0 version ? (I know I should not be asking for ETA, but one time every two month is fair, no ? )

[riemers]

Would be nice, i saw that i needed "solar=1" to get the UV index (its in Beta hence why you can get it like that) but its nice to know intensity for automation.

[cgtobi]

Is this still being worked on?