Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-add local pyown usage with local api-key. Refactored OWNApi into … #123

Open
wants to merge 2 commits into
base: 20.02
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 49 additions & 14 deletions __init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
from pyowm.webapi25.forecaster import Forecaster
from pyowm.webapi25.forecastparser import ForecastParser
from pyowm.webapi25.observationparser import ObservationParser
from pyowm import OWM
from requests import HTTPError, Response

try:
Expand Down Expand Up @@ -80,9 +81,10 @@ class LocationNotFoundError(ValueError):

class OWMApi(Api):
''' Wrapper that defaults to the Mycroft cloud proxy so user's don't need
to get their own OWM API keys '''
to get their own OWM API keys.
However it also handles a local OWM api_key.'''
Comment on lines 83 to +85
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps could rewrite a little eg

Wrapper for the Open Weather Maps API.
By default the Mycroft cloud proxy will be used so user's don't need an API key.
If an api_key is provided, it will connect directly to the OWM API instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you changing that or shall I?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You go for it, I'd have to mess up your PR with a random commit from me


def __init__(self):
def __init__(self, api_key, use_proxy):
super(OWMApi, self).__init__("owm")
self.owmlang = "en"
self.encoding = "utf8"
Expand All @@ -91,6 +93,12 @@ def __init__(self):
self.query_cache = {}
self.location_translations = {}

if api_key and not use_proxy:
self.owm = OWM(api_key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if local_owm or similar would be more descriptive?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I was also thinking about that or calling the other owm owm_wrapper

else:
self.owm = None


@staticmethod
def get_language(lang):
"""
Expand Down Expand Up @@ -137,6 +145,7 @@ def build_query(self, params):

def request(self, data):
""" Caching the responses """
# request is not used for local_api key
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary for this PR, but would be good for us to add caching support for local_api users in the future.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should add a TODO here then

req_hash = hash(json.dumps(data, sort_keys=True))
cache = self.query_cache.get(req_hash, (0, None))
# check for caches with more days data than requested
Expand Down Expand Up @@ -170,6 +179,9 @@ def weather_at_location(self, name):
if name == '':
raise LocationNotFoundError('The location couldn\'t be found')

if self.owm is not None:
return self.owm.weather_at_location(name)

q = {"q": name}
try:
data = self.request({
Expand All @@ -183,12 +195,28 @@ def weather_at_location(self, name):
return self.weather_at_location(name)
raise

def check_for_city_state(self, name):
# someone might ask for denver colorado (which would need to be written denver,colorado)
reg = self.owm.city_id_registry()
if len(reg.ids_for(name)) == 0:
# Try to rewrite name -> replace last blank with comma # TODO: careful this might be language specific -> check
p = name.rfind(" ")
if p>0:
name = name[:p] + "," + name[p+1:]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth rechecking whether this new name variation exists in the registry?
If not we can perhaps return a location not found error earlier.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I am not sure how your location registry actually works and what is in there - so maybe there would be a better solution than this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know much about the registry apart from what is in the pyowm docs

This is a broader problem which is why we added a geolocation endpoint to Mycroft. It's not an easy thing to parse a string and return an exact location. There's always multiple places with the same name, an unknown number of words in each part of the location, and you don't know what combination of parts people are saying eg suburb + country, city + state, city + state + country etc.

Happy to include this in the interim as it does check another variation of possibilities, but ultimately I think we need to move to a complete location parser. Translating the location to coordinates means we can be more confident in the response from OWM.

return name

def weather_at_place(self, name, lat, lon):
if lat and lon:
if self.owm is not None:
return self.owm.weather_at_coords(lat, lon)
q = {"lat": lat, "lon": lon}
else:
if name in self.location_translations:
name = self.location_translations[name]
LOG.info("searching weather for {}".format(name)) # TODO: remove debug
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best to switch this to debug level or remove before we merge. We aim to keep the info log stream as clean as possible.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, wanted to remove that before PR, but maybe keeping it in as debug makes sense? Could even add some more debug logs?

if self.owm is not None:
name = self.check_for_city_state(name)
return self.owm.weather_at_place(name)
response, trans_name = self.weather_at_location(name)
self.location_translations[name] = trans_name
return response
Expand All @@ -201,10 +229,14 @@ def weather_at_place(self, name, lat, lon):

def three_hours_forecast(self, name, lat, lon):
if lat and lon:
if self.owm is not None:
return self.owm.three_hours_forecast_at_coords(lat, lon)
q = {"lat": lat, "lon": lon}
else:
if name in self.location_translations:
name = self.location_translations[name]
if self.owm is not None:
return self.owm.three_hours_forecast(name)
q = {"q": name}

data = self.request({
Expand All @@ -216,6 +248,9 @@ def three_hours_forecast(self, name, lat, lon):
def _daily_forecast_at_location(self, name, limit):
if name in self.location_translations:
name = self.location_translations[name]
if self.owm is not None:
name = self.check_for_city_state(name)
return self.owm.daily_forecast(name, limit)
orig_name = name
while name != '':
try:
Expand All @@ -239,6 +274,8 @@ def _daily_forecast_at_location(self, name, limit):
def daily_forecast(self, name, lat, lon, limit=None):
if lat and lon:
q = {"lat": lat, "lon": lon}
if self.owm is not None:
return self.owm.daily_forecast_at_coords(lat, lon)
else:
return self._daily_forecast_at_location(name, limit)

Expand Down Expand Up @@ -268,6 +305,8 @@ def set_OWM_language(self, lang):
'se': 'latin1'
}
self.encoding = encodings.get(lang, 'utf8')
if self.owm is not None:
self.owm.set_language(lang)


class WeatherSkill(MycroftSkill):
Expand All @@ -288,23 +327,19 @@ def __init__(self):
self.CODES['50d', '50n'] = 7 # windy/misty

# Use Mycroft proxy if no private key provided
self.settings["api_key"] = None
self.settings["use_proxy"] = True
self.settings.setdefault("api_key", None)
self.settings.setdefault("use_proxy", True)

def initialize(self):
# TODO: Remove lat,lon parameters from the OWMApi()
# methods and implement _at_coords() versions
# instead to make the interfaces compatible
# again.
Comment on lines -295 to -298
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I interpreted this as splitting the location methods to make the wrapper interfaces consistent with the OWM API. Though I find the current approach cleaner. A single call for location based queries, and lat, lon would always be considered to be more accurate than a general place name so are prioritised.
Just raising it in case anyone wants to argue the other side before we remove the # TODO

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think having just one call would be more pythonic

#
# if self.settings["api_key"] and not self.settings['use_proxy']):
# self.owm = OWM(self.settings["api_key"])
# else:
# self.owm = OWMApi()
self.owm = OWMApi()
# lat,lon parameters are handled by OWMApi() both for
# local api key and mycrodt's api
Comment on lines +334 to +335
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we agree on this approach, then I don't know that we need this comment here any more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and would also kill the spelling mistake


self.owm = OWMApi( self.settings["api_key"], self.settings['use_proxy'] )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the thinking behind two settings? Sometimes people with their own API key may wish to switch on/off the proxy without adding and removing the key?

Also need to add these settings to the settingsmeta.json

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You explain that to me, I just took the parameters that were handled before - I think the proxy should be handled somehow much more globally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I didn't see that it was just un-commenting some code. This was before my time, not sure what the thinking was.

Personally I think the less settings the better unless it's adding real value, so if there was no need for a separate setting I would leave it at just an api_key. If the custom key is present then use a local OWM connection, else use the proxy.


if self.owm:
self.owm.set_OWM_language(lang=OWMApi.get_language(self.lang))


self.schedule_for_daily_use()
try:
self.mark2_forecast(self.__initialize_report(None))
Expand Down