From 9ca144ac5ae8991c177ba0b0902049cecb7688b0 Mon Sep 17 00:00:00 2001
From: Chris Veilleux <veilleux.chris@gmail.com>
Date: Tue, 11 May 2021 22:49:45 -0500
Subject: [PATCH] applied code review changes

---
 README.md        |   6 +-
 __init__.py      | 252 ++++++++++++++++++++++++++++++-----------------
 skill/api.py     |  13 ++-
 skill/dialog.py  |  35 ++++---
 skill/util.py    |  47 +++++++--
 skill/weather.py |  83 +++++++++++-----
 6 files changed, 289 insertions(+), 147 deletions(-)

diff --git a/README.md b/README.md
index 858f1580..3173c302 100644
--- a/README.md
+++ b/README.md
@@ -5,14 +5,10 @@ Weather conditions and forecasts
 
 ## About 
 Get weather conditions, forecasts, expected precipitation and more!  By default, it will tell
-you about your default location. You can also ask for other cities around the world. 
+you about your device's configured location. You can also ask for other cities around the world. 
 
 Current conditions and weather forecasts come from [Open Weather Map](https://openweathermap.org).
 
-For hardware with Arduino support, like the Mark I, conditions are briefly shown.
-For hardware with GUI support, like the Mark II, there are several screens depending
-on your request.
-
 The temperature is shown in Celsius or Fahrenheit depending on the preferences 
 set in your [https://home.mycroft.ai](https://home.mycroft.ai) account.  You can ask 
 specifically for a unit that differs from your configuration.
diff --git a/__init__.py b/__init__.py
index 801c86f1..9e86ea13 100644
--- a/__init__.py
+++ b/__init__.py
@@ -55,14 +55,6 @@
 
 MARK_II = "mycroft_mark_2"
 TWELVE_HOUR = "half"
-CLEAR = 0
-PARTLY_CLOUDY = 1
-CLOUDY = 2
-LIGHT_RAIN = 3
-RAIN = 4
-THUNDERSTORM = 5
-SNOW = 6
-WINDY = 7
 
 
 class WeatherSkill(MycroftSkill):
@@ -72,12 +64,12 @@ def __init__(self):
         super().__init__("WeatherSkill")
         self.weather_api = OpenWeatherMapApi()
         self.weather_api.set_language_parameter(self.lang)
-        self.weather_config = WeatherConfig(self.config_core, self.settings)
         self.platform = self.config_core["enclosure"].get("platform", "unknown")
+        self.weather_config = None
 
-        # Use Mycroft proxy if no private key provided
-        self.settings["api_key"] = None
-        self.settings["use_proxy"] = True
+    def initialize(self):
+        """Do these things after the skill is loaded."""
+        self.weather_config = WeatherConfig(self.config_core, self.settings)
 
     @intent_handler(
         IntentBuilder("")
@@ -89,7 +81,8 @@ def __init__(self):
     def handle_current_weather(self, message: Message):
         """Handle current weather requests such as: what is the weather like?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_current_weather(message)
 
@@ -103,7 +96,8 @@ def handle_current_weather(self, message: Message):
     def handle_like_outside(self, message: Message):
         """Handle current weather requests such as: what's it like outside?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_current_weather(message)
 
@@ -121,7 +115,8 @@ def handle_number_days_forecast(self, message: Message):
             "What is the 3 day forecast?"
             "What is the weather forecast?"
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         if self.voc_match(message.data["utterance"], "couple"):
             days = 2
@@ -145,7 +140,8 @@ def handle_one_day_forecast(self, message):
             "What is the weather forecast tomorrow?"
             "What is the weather forecast on Tuesday in Baltimore?"
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_one_day_forecast(message)
 
@@ -159,7 +155,8 @@ def handle_one_day_forecast(self, message):
     def handle_weather_later(self, message: Message):
         """Handle future weather requests such as: what's the weather later?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_one_hour_weather(message)
 
@@ -174,7 +171,8 @@ def handle_weather_later(self, message: Message):
     def handle_weather_at_time(self, message: Message):
         """Handle future weather requests such as: what's the weather tonight?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_one_hour_weather(message)
 
@@ -188,7 +186,8 @@ def handle_weather_at_time(self, message: Message):
     def handle_weekend_forecast(self, message: Message):
         """Handle requests for the weekend forecast.
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_weekend_forecast(message)
 
@@ -202,7 +201,8 @@ def handle_weekend_forecast(self, message: Message):
     def handle_week_weather(self, message: Message):
         """Handle weather for week (i.e. seven days).
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_week_summary(message)
 
@@ -222,7 +222,8 @@ def handle_current_temperature(self, message: Message):
             "What is the temperature in Celsius?"
             "What is the temperature in Baltimore now?"
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_temperature(message, temperature_type="current")
 
@@ -239,7 +240,8 @@ def handle_daily_temperature(self, message: Message):
 
         Examples: "What is the temperature?"
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_temperature(message, temperature_type="current")
 
@@ -258,7 +260,8 @@ def handle_hourly_temperature(self, message: Message):
             "What is the temperature tonight?"
             "What is the temperature tomorrow morning?"
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_temperature(message)
 
@@ -280,7 +283,8 @@ def handle_high_temperature(self, message: Message):
             "What is the high temperature tomorrow?"
             "What is the high temperature in London on Tuesday?"
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_temperature(message, temperature_type="high")
 
@@ -302,7 +306,8 @@ def handle_low_temperature(self, message: Message):
             "What is the high temperature tomorrow?"
             "What is the high temperature in London on Tuesday?"
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_temperature(message, temperature_type="low")
 
@@ -316,7 +321,8 @@ def handle_low_temperature(self, message: Message):
     def handle_is_it_hot(self, message: Message):
         """Handler for temperature requests such as: is it going to be hot today?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_temperature(message, "current")
 
@@ -332,7 +338,8 @@ def handle_is_it_hot(self, message: Message):
     def handle_how_hot_or_cold(self, message):
         """Handler for temperature requests such as: how cold will it be today?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         temperature_type = "high" if message.data.get("Hot") else "low"
         self._report_temperature(message, temperature_type)
@@ -347,7 +354,8 @@ def handle_how_hot_or_cold(self, message):
     def handle_is_it_windy(self, message: Message):
         """Handler for weather requests such as: is it windy today?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_wind(message)
 
@@ -362,7 +370,8 @@ def handle_is_it_windy(self, message: Message):
     def handle_windy(self, message):
         """Handler for weather requests such as: how windy is it?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_wind(message)
 
@@ -372,7 +381,8 @@ def handle_windy(self, message):
     def handle_is_it_snowing(self, message: Message):
         """Handler for weather requests such as: is it snowing today?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_weather_condition(message, "snow")
 
@@ -385,7 +395,8 @@ def handle_is_it_snowing(self, message: Message):
     def handle_is_it_clear(self, message: Message):
         """Handler for weather requests such as: is the sky clear today?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_weather_condition(message, condition="clear")
 
@@ -399,7 +410,8 @@ def handle_is_it_clear(self, message: Message):
     def handle_is_it_cloudy(self, message: Message):
         """Handler for weather requests such as: is it cloudy today?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_weather_condition(message, "clouds")
 
@@ -409,7 +421,8 @@ def handle_is_it_cloudy(self, message: Message):
     def handle_is_it_foggy(self, message: Message):
         """Handler for weather requests such as: is it foggy today?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_weather_condition(message, "fog")
 
@@ -419,15 +432,17 @@ def handle_is_it_foggy(self, message: Message):
     def handle_is_it_raining(self, message: Message):
         """Handler for weather requests such as: is it raining today?
 
-        :param message: Message Bus event information from the intent parser
-        """
+        Args:
+            message: Message Bus event information from the intent parser
+0]       """
         self._report_weather_condition(message, "rain")
 
     @intent_handler("do-i-need-an-umbrella.intent")
     def handle_need_umbrella(self, message: Message):
         """Handler for weather requests such as: will I need an umbrella today?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_weather_condition(message, "rain")
 
@@ -440,7 +455,8 @@ def handle_need_umbrella(self, message: Message):
     def handle_is_it_storming(self, message: Message):
         """Handler for weather requests such as:  is it storming today?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         self._report_weather_condition(message, "thunderstorm")
 
@@ -454,7 +470,8 @@ def handle_is_it_storming(self, message: Message):
     def handle_next_precipitation(self, message: Message):
         """Handler for weather requests such as: when will it rain next?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         intent_data = WeatherIntent(message, self.lang)
         weather = self._get_weather(intent_data)
@@ -463,6 +480,10 @@ def handle_next_precipitation(self, message: Message):
             intent_data.timeframe = timeframe
             dialog = WeatherDialog(forecast, self.weather_config, intent_data)
             dialog.build_next_precipitation_dialog()
+            spoken_percentage = self.translate(
+                "percentage-number", data=dict(number=dialog.data["percent"])
+            )
+            dialog.data.update(percent=spoken_percentage)
             self._speak_weather(dialog)
 
     @intent_handler(
@@ -475,7 +496,8 @@ def handle_next_precipitation(self, message: Message):
     def handle_humidity(self, message: Message):
         """Handler for weather requests such as: how humid is it?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         intent_data = self._get_intent_data(message)
         weather = self._get_weather(intent_data)
@@ -485,7 +507,7 @@ def handle_humidity(self, message: Message):
             dialog.build_humidity_dialog()
             dialog.data.update(
                 humidity=self.translate(
-                    "percentage.number", data=dict(num=dialog.data.humidity)
+                    "percentage-number", data=dict(num=dialog.data["humidity"])
                 )
             )
             self._speak_weather(dialog)
@@ -501,7 +523,8 @@ def handle_humidity(self, message: Message):
     def handle_sunrise(self, message: Message):
         """Handler for weather requests such as: when is the sunrise?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         intent_data = self._get_intent_data(message)
         weather = self._get_weather(intent_data)
@@ -510,9 +533,8 @@ def handle_sunrise(self, message: Message):
             dialog_args = intent_data, self.weather_config, intent_weather
             dialog = get_dialog_for_timeframe(intent_data.timeframe, dialog_args)
             dialog.build_sunrise_dialog()
-            if self.platform == MARK_II:
-                weather_location = self._build_display_location(intent_data)
-                self._display_sunrise_sunset_mark_ii(intent_weather, weather_location)
+            weather_location = self._build_display_location(intent_data)
+            self._display_sunrise_sunset(intent_weather, weather_location)
             self._speak_weather(dialog)
 
     @intent_handler(
@@ -526,7 +548,8 @@ def handle_sunrise(self, message: Message):
     def handle_sunset(self, message: Message):
         """Handler for weather requests such as: when is the sunset?
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         intent_data = self._get_intent_data(message)
         weather = self._get_weather(intent_data)
@@ -535,17 +558,30 @@ def handle_sunset(self, message: Message):
             dialog_args = intent_data, self.weather_config, intent_weather
             dialog = get_dialog_for_timeframe(intent_data.timeframe, dialog_args)
             dialog.build_sunset_dialog()
-            if self.platform == MARK_II:
-                weather_location = self._build_display_location(intent_data)
-                self._display_sunrise_sunset_mark_ii(intent_weather, weather_location)
+            weather_location = self._build_display_location(intent_data)
+            self._display_sunrise_sunset(intent_weather, weather_location)
             self._speak_weather(dialog)
 
-    def _display_sunrise_sunset_mark_ii(
+    def _display_sunrise_sunset(
         self, forecast: DailyWeather, weather_location: str
     ):
         """Display the sunrise and sunset.
 
-        :param forecast: daily forecasts to display
+        Args:
+            forecast: daily forecasts to display
+            weather_location: the geographical location of the weather
+        """
+        if self.platform == MARK_II:
+            self._display_sunrise_sunset_mark_ii(forecast, weather_location)
+
+    def _display_sunrise_sunset_mark_ii(
+        self, forecast: DailyWeather, weather_location: str
+    ):
+        """Display the sunrise and sunset on a Mark II device using a grid layout.
+
+        Args:
+            forecast: daily forecasts to display
+            weather_location: the geographical location of the weather
         """
         self.gui.clear()
         self.gui["weatherDate"] = forecast.date_time.strftime("%A %b %d")
@@ -561,8 +597,11 @@ def _format_sunrise_sunset_time(self, date_time: datetime) -> str:
         The datetime builtin returns hour in two character format.  Remove the
         leading zero when present.
 
-        :param date_time: the sunrise or sunset
-        :return: the value to display on the screen.
+        Args:
+            date_time: the sunrise or sunset
+
+        Returns:
+            the value to display on the screen
         """
         if self.config_core["time_format"] == TWELVE_HOUR:
             display_time = date_time.strftime("%I:%M")
@@ -576,7 +615,8 @@ def _format_sunrise_sunset_time(self, date_time: datetime) -> str:
     def _report_current_weather(self, message: Message):
         """Handles all requests for current weather conditions.
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         intent_data = self._get_intent_data(message)
         weather = self._get_weather(intent_data)
@@ -607,7 +647,9 @@ def _display_current_conditions(
 
         This is the first screen that shows.  Others will follow.
 
-        :param weather: current weather conditions from Open Weather Maps
+        Args:
+            weather: current weather conditions from Open Weather Maps
+            weather_location: the geographical location of the reported weather
         """
         if self.gui.connected:
             page_name = "current_1_scalable.qml"
@@ -637,8 +679,11 @@ def _build_display_location(self, intent_data: WeatherIntent) -> str:
         region.  A specified location in a different country will result in a return
         value of city and country.
 
-        :param intent_data: information about the intent that was triggered
-        :return: The weather location to be displayed on the GUI
+        Args:
+            intent_data: information about the intent that was triggered
+
+        Returns:
+            The weather location to be displayed on the GUI
         """
         if intent_data.geolocation:
             location = [intent_data.geolocation["city"]]
@@ -658,7 +703,9 @@ def _display_more_current_conditions(
 
         This is the second screen that shows for current weather.
 
-        :param weather: current weather conditions from Open Weather Maps
+        Args
+            weather: current weather conditions from Open Weather Maps
+            weather_location: geographical location of the reported weather
         """
         page_name = "current_2_scalable.qml"
         self.gui.clear()
@@ -675,7 +722,8 @@ def _display_more_current_conditions(
     def _report_one_hour_weather(self, message: Message):
         """Handles requests for a one hour forecast.
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         intent_data = self._get_intent_data(message)
         weather = self._get_weather(intent_data)
@@ -727,7 +775,8 @@ def _display_hourly_forecast(self, weather: WeatherReport, weather_location: str
     def _report_one_day_forecast(self, message: Message):
         """Handles all requests for a single day forecast.
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         intent_data = WeatherIntent(message, self.lang)
         weather = self._get_weather(intent_data)
@@ -776,7 +825,8 @@ def _report_multi_day_forecast(self, message: Message, days: int):
     def _report_weekend_forecast(self, message: Message):
         """Handles requests for a weekend forecast.
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         intent_data = self._get_intent_data(message)
         weather = self._get_weather(intent_data)
@@ -811,7 +861,8 @@ def _report_week_summary(self, message: Message):
         When the user requests the weather for the week, rather than give a daily
         forecast for seven days, summarize the weather conditions for the week.
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         intent_data = WeatherIntent(message, self.lang)
         weather = self._get_weather(intent_data)
@@ -828,19 +879,18 @@ def _build_weekly_condition_dialogs(
     ) -> List[WeeklyDialog]:
         """Build the dialog communicating a weather condition on days it is forecasted.
 
-        :param forecast: seven day daily forecast
-        :param intent_data: Parsed intent data
-        :return: List of dialogs for each condition expected in the coming week.
+        Args:
+            forecast: seven day daily forecast
+            intent_data: Parsed intent data
+
+        Returns:
+            List of dialogs for each condition expected in the coming week.
         """
         dialogs = list()
         conditions = set([daily.condition.category for daily in forecast])
         for condition in conditions:
             dialog = WeeklyDialog(intent_data, self.weather_config, forecast)
             dialog.build_condition_dialog(condition=condition)
-            dialog.data.update(
-                condition=self.translate(condition.lower()),
-                days=dialog.data["days"].replace("and", self.translate("and")),
-            )
             dialogs.append(dialog)
 
         return dialogs
@@ -850,9 +900,12 @@ def _build_weekly_temperature_dialog(
     ) -> WeeklyDialog:
         """Build the dialog communicating the forecasted range of temperatures.
 
-        :param forecast: seven day daily forecast
-        :param intent_data: Parsed intent data
-        :return: Dialog for the temperature ranges over the coming week.
+        Args:
+            forecast: seven day daily forecast
+            intent_data: Parsed intent data
+
+        Returns:
+            Dialog for the temperature ranges over the coming week.
         """
         dialog = WeeklyDialog(intent_data, self.weather_config, forecast)
         dialog.build_temperature_dialog()
@@ -864,7 +917,9 @@ def _display_multi_day_forecast(
     ):
         """Display daily forecast data on devices that support the GUI.
 
-        :param forecast: daily forecasts to display
+        Args:
+            forecast: daily forecasts to display
+            intent_data: Parsed intent data
         """
         if self.platform == MARK_II:
             self._display_multi_day_mark_ii(forecast, intent_data)
@@ -878,7 +933,9 @@ def _display_multi_day_mark_ii(
 
         The Mark II supports displaying four days of a forecast at a time.
 
-        :param forecast: daily forecasts to display
+        Args:
+            forecast: daily forecasts to display
+            intent_data: Parsed intent data
         """
         page_name = "daily_mark_ii.qml"
         daily_forecast = []
@@ -906,7 +963,8 @@ def _display_multi_day_scalable(self, forecast: List[DailyWeather]):
 
         The generic layout supports displaying two days of a forecast at a time.
 
-        :param forecast: daily forecasts to display
+        Args:
+            forecast: daily forecasts to display
         """
         page_one_name = "daily_1_scalable.qml"
         page_two_name = page_one_name.replace("1", "2")
@@ -930,8 +988,9 @@ def _display_multi_day_scalable(self, forecast: List[DailyWeather]):
     def _report_temperature(self, message: Message, temperature_type: str = None):
         """Handles all requests for a temperature.
 
-        :param message: Message Bus event information from the intent parser
-        :param temperature_type: current, high or low temperature
+        Args:
+            message: Message Bus event information from the intent parser
+            temperature_type: current, high or low temperature
         """
         intent_data = self._get_intent_data(message)
         weather = self._get_weather(intent_data)
@@ -945,8 +1004,9 @@ def _report_temperature(self, message: Message, temperature_type: str = None):
     def _report_weather_condition(self, message: Message, condition: str):
         """Handles all requests for a specific weather condition.
 
-        :param message: Message Bus event information from the intent parser
-        :param condition: the weather condition specified by the user
+        Args:
+            message: Message Bus event information from the intent parser
+            condition: the weather condition specified by the user
         """
         intent_data = self._get_intent_data(message)
         weather = self._get_weather(intent_data)
@@ -962,9 +1022,10 @@ def _build_condition_dialog(
     ) -> WeatherDialog:
         """Builds a dialog for the requested weather condition.
 
-        :param weather: Current, hourly or daily weather forecast
-        :param intent_data: Parsed intent data
-        :param condition: weather condition requested by the user
+        Args:
+            weather: Current, hourly or daily weather forecast
+            intent_data: Parsed intent data
+            condition: weather condition requested by the user
         """
         dialog_args = intent_data, self.weather_config, weather
         dialog = get_dialog_for_timeframe(intent_data.timeframe, dialog_args)
@@ -977,7 +1038,8 @@ def _build_condition_dialog(
     def _report_wind(self, message: Message):
         """Handles all requests for a wind conditions.
 
-        :param message: Message Bus event information from the intent parser
+        Args:
+            message: Message Bus event information from the intent parser
         """
         intent_data = self._get_intent_data(message)
         weather = self._get_weather(intent_data)
@@ -991,10 +1053,13 @@ def _report_wind(self, message: Message):
             self._speak_weather(dialog)
 
     def _get_intent_data(self, message: Message) -> WeatherIntent:
-        """Parse the intend data from the message into data used in the skill.
+        """Parse the intent data from the message into data used in the skill.
 
-        :param message: Message Bus event information from the intent parser
-        :return: parsed information about the intent
+        Args:
+            message: Message Bus event information from the intent parser
+
+        Returns:
+            parsed information about the intent
         """
         intent_data = None
         try:
@@ -1015,8 +1080,11 @@ def _get_intent_data(self, message: Message) -> WeatherIntent:
     def _get_weather(self, intent_data: WeatherIntent) -> WeatherReport:
         """Call the Open Weather Map One Call API to get weather information
 
-        :param intent_data: Parsed intent data
-        :return: An object representing the data returned by the API
+        Args:
+            intent_data: Parsed intent data
+
+        Returns:
+            An object representing the data returned by the API
         """
         weather = None
         if intent_data is not None:
@@ -1042,7 +1110,8 @@ def _get_weather(self, intent_data: WeatherIntent) -> WeatherReport:
     def _handle_api_error(self, exception: HTTPError):
         """Communicate an error condition to the user.
 
-        :param exception: the HTTPError returned by the API call
+        Args:
+            exception: the HTTPError returned by the API call
         """
         if exception.response.status_code == 401:
             self.bus.emit(Message("mycroft.not.paired"))
@@ -1054,8 +1123,11 @@ def _determine_weather_location(
     ) -> Tuple[float, float]:
         """Determine latitude and longitude using the location data in the intent.
 
-        :param intent_data: Parsed intent data
-        :return: latitude and longitude of the location
+        Args:
+            intent_data: Parsed intent data
+
+        Returns
+            latitude and longitude of the location
         """
         if intent_data.location is None:
             latitude = self.weather_config.latitude
diff --git a/skill/api.py b/skill/api.py
index 2efbc74e..1032eed4 100644
--- a/skill/api.py
+++ b/skill/api.py
@@ -89,9 +89,10 @@ def get_weather_for_coordinates(
     ) -> WeatherReport:
         """Issue an API call and map the return value into a weather report
 
-        :param measurement_system: Metric or Imperial measurement units
-        :param latitude: the geologic latitude of the weather location
-        :param longitude: the geologic longitude of the weather location
+        Args:
+            measurement_system: Metric or Imperial measurement units
+            latitude: the geologic latitude of the weather location
+            longitude: the geologic longitude of the weather location
         """
         query_parameters = dict(
             exclude="minutely",
@@ -109,7 +110,11 @@ def get_weather_for_coordinates(
     def set_language_parameter(self, language_config: str):
         """
         OWM supports 31 languages, see https://openweathermap.org/current#multi
-        Convert language code to owm language, if missing use 'en'
+
+        Convert Mycroft's language code to OpenWeatherMap's, if missing use english.
+
+        Args:
+            language_config: The Mycroft language code.
         """
         special_cases = {"cs": "cz", "ko": "kr", "lv": "la"}
         language_part_one, language_part_two = language_config.split('-')
diff --git a/skill/dialog.py b/skill/dialog.py
index cbec3465..79cdddbd 100644
--- a/skill/dialog.py
+++ b/skill/dialog.py
@@ -11,10 +11,30 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-"""Abstraction of dialog building for the weather skill."""
+"""Abstraction of dialog building for the weather skill.
+
+There are A LOT of dialog files in this skill.  All the permutations of timeframe,
+weather condition and location add up fast.  To help with discoverability, a naming
+convention was applied to the dialog files:
+    <timeframe>-<weather info>-<qualifier>-<locale>.dialog
+
+    Example:
+         daily-temperature-high-local.dialog
+
+    * Timeframe: the date or time applicable to the forecast.  This skill supports
+        current, hourly and daily weather.
+    * Weather info: a description of what type of weather the dialog refers to.
+        Examples include "temperature", "weather" and "sunrise".
+    * Qualifier: further qualifies what type of weather is being reported.  For
+        example, temperature can be qualified by "high" or "low".
+    * Locale: indicates if the dialog is for local weather or weather in a remote
+        location.
+
+The skill class will use the "name" and "data" attributes to pass to the TTS process.
+"""
 from typing import List, Tuple
 
-from mycroft.util.format import nice_number, nice_time
+from mycroft.util.format import join_list, nice_number, nice_time
 from mycroft.util.time import now_local
 from .config import WeatherConfig
 from .intent import WeatherIntent
@@ -26,9 +46,6 @@
     DailyWeather,
     HOURLY,
     HourlyWeather,
-    is_current_weather,
-    is_hourly_forecast,
-    is_daily_forecast,
 )
 
 # TODO: MISSING DIALOGS
@@ -77,9 +94,6 @@ def __init__(self, weather, config, intent_data):
         self.weather = weather
         self.config = config
         self.intent_data = intent_data
-        self.current_weather = is_current_weather(weather)
-        self.daily_forecast = is_daily_forecast(weather)
-        self.hourly_forecast = is_hourly_forecast(weather)
 
     def build_wind_dialog(self):
         """Build the components necessary to speak the wind conditions."""
@@ -378,10 +392,7 @@ def build_condition_dialog(self, condition: str):
             if daily.condition.category == condition:
                 day = get_speakable_day_of_week(daily.date_time)
                 days_with_condition.append(day)
-        if len(days_with_condition) > 1:
-            last_day = days_with_condition.pop(-1)
-            days_with_condition.append("and " + last_day)
-        self.data.update(days=", ".join(days_with_condition))
+        self.data.update(days=join_list(days_with_condition, "and"))
 
 
 def get_dialog_for_timeframe(timeframe: str, dialog_ags: Tuple):
diff --git a/skill/util.py b/skill/util.py
index e5168044..8176ec04 100644
--- a/skill/util.py
+++ b/skill/util.py
@@ -35,8 +35,12 @@ def convert_to_local_datetime(timestamp: time, timezone: str) -> datetime:
     This function assumes it is passed a timestamp in the UTC timezone.  It
     then adjusts the datetime to match the specified timezone.
 
-    :param timestamp: seconds since epoch
-    :param timezone: the timezone requested by the user
+    Args:
+        timestamp: seconds since epoch
+        timezone: the timezone requested by the user
+
+    Returns:
+        A datetime in the passed timezone based on the passed timestamp
     """
     naive_datetime = datetime.fromtimestamp(timestamp)
     utc_datetime = pytz.utc.localize(naive_datetime)
@@ -51,9 +55,13 @@ def get_utterance_datetime(
 ) -> datetime:
     """Get a datetime representation of a date or time concept in an utterance.
 
-    :param utterance: the words spoken by the user
-    :param timezone: the timezone requested by the user
-    :param language: the language configured on the device
+    Args:
+        utterance: the words spoken by the user
+        timezone: the timezone requested by the user
+        language: the language configured on the device
+
+    Returns:
+        The date and time represented in the utterance in the specified timezone.
     """
     utterance_datetime = None
     if timezone is None:
@@ -71,7 +79,11 @@ def get_utterance_datetime(
 def get_tz_info(timezone: str) -> tzinfo:
     """Generate a tzinfo object from a timezone string.
 
-    :param timezone: a string representing a timezone
+    Args:
+        timezone: a string representing a timezone
+
+    Returns:
+        timezone in a string format
     """
     return pytz.timezone(timezone)
 
@@ -79,7 +91,15 @@ def get_tz_info(timezone: str) -> tzinfo:
 def get_geolocation(location: str):
     """Retrieve the geolocation information about the requested location.
 
-    :param location: a location specified in the utterance
+    Args:
+        location: a location specified in the utterance
+
+    Returns:
+        A deserialized JSON object containing geolocation information for the
+        specified city.
+
+    Raises:
+        LocationNotFound error if the API returns no results.
     """
     geolocation_api = GeolocationApi()
     geolocation = geolocation_api.get_geolocation(location)
@@ -93,7 +113,11 @@ def get_geolocation(location: str):
 def get_time_period(intent_datetime: datetime) -> str:
     """Translate a specific time '9am' to period of the day 'morning'
 
-    :param intent_datetime: the datetime extracted from an utterance
+    Args:
+        intent_datetime: the datetime extracted from an utterance
+
+    Returns:
+        A generalized time of day based on the passed datetime object.
     """
     hour = intent_datetime.time().hour
     if 1 <= hour < 5:
@@ -113,8 +137,11 @@ def get_time_period(intent_datetime: datetime) -> str:
 def get_speakable_day_of_week(date_to_speak: datetime):
     """Convert the time of the a daily weather forecast to a speakable day of week.
 
-    :param date_to_speak: The date/time for the forecast being reported.
-    :return: The day of the week in the device's configured language
+    Args:
+        date_to_speak: The date/time for the forecast being reported.
+
+    Returns:
+        The day of the week in the device's configured language
     """
     now = now_local()
     tomorrow = now.date() + timedelta(days=1)
diff --git a/skill/weather.py b/skill/weather.py
index edf4cf08..4fbe5e3e 100644
--- a/skill/weather.py
+++ b/skill/weather.py
@@ -14,6 +14,7 @@
 """Representations and conversions of the data returned by the weather API."""
 from datetime import timedelta
 from pathlib import Path
+from typing import List
 
 from .config import MILES_PER_HOUR
 from .util import convert_to_local_datetime
@@ -79,21 +80,6 @@
 )
 
 
-def is_daily_forecast(weather_report) -> bool:
-    """Helper function to determine if the object passed is a DailyWeather object."""
-    return isinstance(weather_report, DailyWeather)
-
-
-def is_hourly_forecast(weather_report) -> bool:
-    """Helper function to determine if the object passed is a HourlyWeather object."""
-    return isinstance(weather_report, HourlyWeather)
-
-
-def is_current_weather(weather_report) -> bool:
-    """Helper function to determine if the object passed is a CurrentWeather object."""
-    return isinstance(weather_report, CurrentWeather)
-
-
 class WeatherCondition:
     """Data representation of a weather conditions JSON object from the API"""
 
@@ -152,7 +138,11 @@ def __init__(self, weather: dict, timezone: str):
     def _determine_wind_direction(degree_direction: int):
         """Convert wind direction from compass degrees to compass direction.
 
-        :param degree_direction: Degrees on a compass indicating wind direction.
+        Args:
+            degree_direction: Degrees on a compass indicating wind direction.
+
+        Returns:
+            the wind direction in one of eight compass directions
         """
         for min_degree, compass_direction in WIND_DIRECTION_CONVERSION:
             if degree_direction < min_degree:
@@ -166,7 +156,11 @@ def _determine_wind_direction(degree_direction: int):
     def determine_wind_strength(self, speed_unit: str):
         """Convert a wind speed to a wind strength.
 
-        :param speed_unit: unit of measure for speed depending on device configuration
+        Args:
+            speed_unit: unit of measure for speed depending on device configuration
+
+        Returns:
+            a string representation of the wind strength
         """
         if speed_unit == MILES_PER_HOUR:
             limits = dict(strong=20, moderate=11)
@@ -263,7 +257,11 @@ def __init__(self, report):
             self.alerts = None
 
     def get_weather_for_intent(self, intent_data):
-        """Use the intent to determine which forecast satisfies the request"""
+        """Use the intent to determine which forecast satisfies the request.
+
+        Args:
+            intent_data: Parsed intent data
+        """
         if intent_data.timeframe == "hourly":
             weather = self.get_forecast_for_hour(intent_data)
         elif intent_data.timeframe == "daily":
@@ -274,7 +272,11 @@ def get_weather_for_intent(self, intent_data):
         return weather
 
     def get_forecast_for_date(self, intent_data):
-        """Use the intent to determine which daily forecast(s) satisfies the request"""
+        """Use the intent to determine which daily forecast(s) satisfies the request.
+
+        Args:
+            intent_data: Parsed intent data
+        """
         if intent_data.intent_datetime.date() == intent_data.location_datetime.date():
             forecast = self.daily[0]
         else:
@@ -285,8 +287,18 @@ def get_forecast_for_date(self, intent_data):
 
         return forecast
 
-    def get_forecast_for_multiple_days(self, days):
-        """Use the intent to determine which daily forecast(s) satisfies the request"""
+    def get_forecast_for_multiple_days(self, days: int) -> List[DailyWeather]:
+        """Use the intent to determine which daily forecast(s) satisfies the request.
+
+        Args:
+            days: number of forecast days to return
+
+        Returns:
+            list of daily forecasts for the specified number of days
+
+        Raises:
+            IndexError when number of days is more than what is returned by the API
+        """
         if days > 7:
             raise IndexError("Only seven days of forecasted weather available.")
 
@@ -295,7 +307,14 @@ def get_forecast_for_multiple_days(self, days):
         return forecast
 
     def get_forecast_for_hour(self, intent_data):
-        """Use the intent to determine which hourly forecast(s) satisfies the request"""
+        """Use the intent to determine which hourly forecast(s) satisfies the request.
+
+        Args:
+            intent_data: Parsed intent data
+
+        Returns:
+            A single hour of forecast data based on the intent data
+        """
         delta = intent_data.intent_datetime - intent_data.location_datetime
         hour_delta = int(delta / timedelta(hours=1))
         hour_index = hour_delta + 1
@@ -304,7 +323,11 @@ def get_forecast_for_hour(self, intent_data):
         return report
 
     def get_weekend_forecast(self):
-        """Use the intent to determine which daily forecast(s) satisfies the request"""
+        """Use the intent to determine which daily forecast(s) satisfies the request.
+
+        Returns:
+            The Saturday and Sunday forecast from the list of daily forecasts
+        """
         forecast = list()
         for forecast_day in self.daily:
             report_date = forecast_day.date_time.date()
@@ -314,10 +337,18 @@ def get_weekend_forecast(self):
         return forecast
 
     def get_next_precipitation(self, intent_data):
-        """Determine when the next chance of precipitation is in the forecast"""
+        """Determine when the next chance of precipitation is in the forecast.
+
+        Args:
+            intent_data: Parsed intent data
+
+        Returns:
+            The weather report containing the next chance of rain and the timeframe of
+            the selected report.
+        """
         report = None
         current_precipitation = True
-        timeframe = "hourly"
+        timeframe = HOURLY
         for hourly in self.hourly:
             if hourly.date_time.date() > intent_data.location_datetime.date():
                 break
@@ -329,7 +360,7 @@ def get_next_precipitation(self, intent_data):
                 current_precipitation = False
 
         if report is None:
-            timeframe = "daily"
+            timeframe = DAILY
             for daily in self.daily:
                 if daily.date_time.date() == intent_data.location_datetime.date():
                     continue