Merge lp:~samuel-buffet/entertainer/bug_379824 into lp:entertainer

[Samuel Buffet]

Weather conditions download from Google are more robust. (Fixes Bug 379824)

[Samuel Buffet (samuel-buffet)]

Devs,

This is a fix I propose for the linked bug.

The bug was caused by a change made by google in the format of the returned result.

They've changed the way they give us the condition pictures.

Before we received things like that : '/images/weather/sunny.gif'.

And now we receive : '/ig/images/weather/sunny.gif'.

So I've fixed the code by spliting the result to only consider the short filename (without the relative path).

This will protect us to any change in the relative path.

I've added a tests also to get conditions from different cities on earth. This is to prevent from future change on short filenames this time.

Samuel-

[Matt Layman (mblayman)]

Samuel,

`make check` was good. The new tests look good and the code is good. I have a couple of minor comments but I'm approving.

1) The init location parameter isn't connected to the self._location instance variable. This means that if refresh() is called before the set_location method is used, then it will try to get the location of ''.

2) The test method docstrings could use a full stop.

3) set_location and get_location don't do much. Could you get rid of the getter and setter and just use self.location?

4) The branch needs a commit message.

review: Approve

[Samuel Buffet (samuel-buffet)]

Matt,

Branch updated an pushed.

incremental diff :

=== modified file 'entertainerlib/frontend/gui/screens/weather.py'
--- entertainerlib/frontend/gui/screens/weather.py 2009-05-09 17:03:51 +0000
+++ entertainerlib/frontend/gui/screens/weather.py 2009-05-25 16:43:06 +0000
@@ -20,11 +20,11 @@
         screen_title = Label(0.13, "screentitle", 0, 0.87, _("Weather"))
         self.add(screen_title)

- if (self.weather.get_location() == ''):
+ if (self.weather.location == ''):
             self.add(Label(0.04167, "text", 0.30, 0.20,
                 _("No weather location defined!"), font_weight="bold"))
         else:
- location_text = self.weather.get_location()
+ location_text = self.weather.location

             location = Label(0.04167, "text", 0.40, 0.13, location_text,
                 font_weight="bold")

=== modified file 'entertainerlib/tests/test_weather.py'
--- entertainerlib/tests/test_weather.py 2009-05-23 21:08:16 +0000
+++ entertainerlib/tests/test_weather.py 2009-05-25 16:46:12 +0000
@@ -74,7 +74,7 @@
                     return ZERO
         england = EnglandTimeZone()

- self.weather.set_location('Bath,England')
+ self.weather.location = 'Bath,England'
         self.weather.refresh()
         forecasts = self.weather.get_forecasts()
         today = forecasts[0]
@@ -93,57 +93,52 @@
         self.assertEqual(today["High"], 20)
         self.assertEqual(today["Condition"],"Sunny")

- def testWeatherSetLocation(self):
- """Tests the location setting of the weather module"""
- self.weather.set_location('Bradford,England')
- self.assertEqual(self.weather.get_location(), 'Bradford,England')
-
     def testWeatherLocationLatinEncoding(self):
         """Tests whether code can handle Latin-1 encoding back from google"""
         #Check for Montreal which gets info back from google in Latin-1
- self.weather.set_location("Montreal")
+ self.weather.location = "Montreal"
         self.weather.refresh()
         results = self.weather.get_forecasts()
         #if there are results then it's working
         self.assertTrue(len(results) > 0)

     def test_yuma_conditions(self):
- """Tests that there's no 'weather-na' in Yuma"""
+ """Tests that there's no 'weather-na' in Yuma."""
         # Here we expect sun http://en.wikipedia.org/wiki/Yuma,_Arizona.
         # `Yuma is the sunniest place on earth` (90% of the time)
- self.weather.set_location("Yuma")
+ self.weather.location = "Yuma"
         self.weather.refresh()
         forecasts = self.weather.get_forecasts()
         for day in forecasts:
             self.assertFalse(day["Image"] == 'weather-na')

     def test_london_conditions(self):
- """Tests that there's no 'weather-na' in London"""
- self.weather.set_location("London")
+ """Tests that there's no 'weather-na' in London."""
+ self.weather.location = "London"
         self.weather.refresh()
         forecasts = self.weather.get_forecasts()
         for day in forecasts:
             self.assertFalse(day["Image"] == 'weather-na')

     def test_perth_conditions(self):
- ""...

[lp:~samuel-buffet/entertainer/bug_379824]

380. By Samuel Buffet on 2009-05-25

Fixes after Matt's comments.

=== modified file 'entertainerlib/tests/test_weather.py'
--- entertainerlib/tests/test_weather.py	2009-05-09 17:03:51 +0000
+++ entertainerlib/tests/test_weather.py	2009-05-23 21:08:16 +0000
@@ -106,3 +106,46 @@
         results = self.weather.get_forecasts()
         #if there are results then it's working
         self.assertTrue(len(results) > 0)
+
+    def test_yuma_conditions(self):
+        """Tests that there's no 'weather-na' in Yuma"""
+        # Here we expect sun http://en.wikipedia.org/wiki/Yuma,_Arizona.
+        # `Yuma is the sunniest place on earth` (90% of the time)
+        self.weather.set_location("Yuma")
+        self.weather.refresh()
+        forecasts = self.weather.get_forecasts()
+        for day in forecasts:
+            self.assertFalse(day["Image"] == 'weather-na')
+
+    def test_london_conditions(self):
+        """Tests that there's no 'weather-na' in London"""
+        self.weather.set_location("London")
+        self.weather.refresh()
+        forecasts = self.weather.get_forecasts()
+        for day in forecasts:
+            self.assertFalse(day["Image"] == 'weather-na')
+
+    def test_perth_conditions(self):
+        """Tests that there's no 'weather-na' in Perth"""
+        self.weather.set_location("Perth")
+        self.weather.refresh()
+        forecasts = self.weather.get_forecasts()
+        for day in forecasts:
+            self.assertFalse(day["Image"] == 'weather-na')
+
+    def test_nyc_conditions(self):
+        """Tests that there's no 'weather-na' in New York"""
+        self.weather.set_location("New York")
+        self.weather.refresh()
+        forecasts = self.weather.get_forecasts()
+        for day in forecasts:
+            self.assertFalse(day["Image"] == 'weather-na')
+
+    def test_north_pole_conditions(self):
+        """Tests that there's no 'weather-na' in North Pole"""
+        self.weather.set_location("North Pole")
+        self.weather.refresh()
+        forecasts = self.weather.get_forecasts()
+        for day in forecasts:
+            self.assertFalse(day["Image"] == 'weather-na')
+

=== modified file 'entertainerlib/utils/weather.py'
--- entertainerlib/utils/weather.py	2009-04-28 22:45:31 +0000
+++ entertainerlib/utils/weather.py	2009-05-23 21:08:16 +0000
@@ -6,6 +6,7 @@
 """
 
 import locale
+import os
 import urllib2
 from xml.dom import minidom
 
@@ -14,21 +15,16 @@
 #The lookup url for the google weather API
 WEATHER_LOOKUP_URL = "http://www.google.co.uk/ig/api?weather="
 
-#use the icon instead of condition to make it work with any languages
-WEATHER_IMAGES = {'/images/weather/mostly_sunny.gif': 'weather-sun-mostly',
-                  '/images/weather/chance_of_rain.gif': 'weather-rain-chance',
-                  #'/images/weather/partly_sunny.gif': 'weather-sun-clouds', 
-                  # no dedicated gif provided by google for that weather 
-                  # condition.
-                  # it's a *mostly_sunny* gif wich is given instead :(
-                  '/images/weather/rain.gif': 'weather-rain',
-                  '/images/weather/storm.gif': 'weather-lightning',
-                  '/images/weather/fog.gif': 'weather-fog',
-                  '/images/weather/sunny.gif': 'weather-sun',
-                  '/images/weather/icy.gif': 'weather-ice',
-                  '/images/weather/cloudy.gif': 'weather-clouds',
-                  '/images/weather/snow.gif': 'weather-snow'
-                  }
+WEATHER_IMAGES = {'mostly_sunny.gif': 'weather-sun-mostly',
+                  'chance_of_rain.gif': 'weather-rain-chance',
+                  'partly_cloudy.gif': 'weather-sun-clouds',
+                  'rain.gif': 'weather-rain',
+                  'storm.gif': 'weather-lightning',
+                  'fog.gif': 'weather-fog',
+                  'sunny.gif': 'weather-sun',
+                  'icy.gif': 'weather-ice',
+                  'cloudy.gif': 'weather-clouds',
+                  'snow.gif': 'weather-snow'}
 
 class Weather:
     """
@@ -36,20 +32,15 @@
     """
 
     def __init__(self, location=""):
-        """
-        Sets up the weather object
-        @param location (String) location of weather to get forecast
-        for.
-        """
-        self.__location = ''
-        self.__forecasts = []
+        self._location = ''
+        self._forecasts = []
         self.theme = Configuration().theme
         self.set_location(location)
-        self.locale = self.__get_locale ()
+        self.locale = self._get_locale()
         self.refresh()
 
-    def __get_locale (self):
-        '''Get locale information from user\'s machine'''
+    def _get_locale(self):
+        """Get locale information from user's machine."""
         default = locale.getdefaultlocale ()[0]
         if default:
             lang = default.split("_")[0]
@@ -59,21 +50,17 @@
         return lang
 
     def find_forecast(self, search):
-        """
-        Returns the search results page for the search
-        @param search String
-        @return array of forecasts
-        """
+        """Returns the search results page for the search."""
         url = WEATHER_LOOKUP_URL + search + "&hl=" + self.locale
         url = url.replace(" ", "%20")
 
         raw_data = urllib2.urlopen(url)
-        # We get the charset from the content-type of the http answer
+        # We get the charset from the content-type of the http answer.
         conent_type = raw_data.info()["Content-Type"]
         encoding = conent_type.split("=")[1]
 
-        # we read data according to the relevant charset and then we
-        # prepare it to be parsed by minidom
+        # We read data according to the relevant charset and then we
+        # prepare it to be parsed by minidom.
         data = unicode(raw_data.read(), encoding)
         data = data.encode("utf8")
         dom = minidom.parseString(data)
@@ -107,11 +94,11 @@
                 if node.nodeName == 'condition':
                     condition = node.getAttribute('data')
                 if node.nodeName == 'icon':
-                    imagename = node.getAttribute('data')
-                    image = self.set_image (imagename)
+                    imagename = os.path.split(node.getAttribute('data'))[-1]
+                    image = self.set_image(imagename)
 
-            # condition is the last element of the forecast we are
-            # interested in so write out the forecast if this is set.
+                # Condition is the last element of the forecast we are
+                # interested in so write out the forecast if this is set.
                 if (condition):
                     forecast = {"Day":day,
                                 "Low":str(converted_low),
@@ -127,48 +114,30 @@
                     imagename = ''
                     image = ''
 
-        return self.__forecasts
+        return self._forecasts
 
     def set_location(self, location):
-        """
-        Sets the current location
-        @param type String should be a city, country name or
-        post/zip code
-        """
-        self.__location = location
+        """Sets the current location."""
+        self._location = location
 
     def get_location(self):
-        """
-        Gets the current location
-        @return type String of city, country name or post/zip code
-        """
-        return self.__location
+        """Gets the current location."""
+        return self._location
 
     def clear_forecasts(self):
-        """
-        Resets the forecast array
-        """
-        self.__forecasts = []
+        """Resets the forecast array."""
+        self._forecasts = []
 
     def add_forecast(self, forecast):
-        """
-        Appends a forecast to the forecast array
-        @param forecast dict
-        """
-        self.__forecasts.append(forecast)
+        """Appends a forecast to the forecast array."""
+        self._forecasts.append(forecast)
 
     def get_forecasts(self):
-        """
-        Returns an array of forecasts
-        @return array of forecasts
-        """
-        return self.__forecasts
+        """Returns an array of forecasts."""
+        return self._forecasts
 
     def set_image(self, condition):
-        """
-        Returns an image for a given weather condition.
-        @return type String of weather image
-        """
+        """Returns an image for a given weather condition."""
         try:
             image_name = WEATHER_IMAGES[condition]
         except KeyError:
@@ -179,11 +148,8 @@
         return image
 
     def refresh(self):
-        """
-        Clear current weather and forecasts and then loads new data
-        from xml or rss feed
-        """
-        if (self.__location):
+        """Clear current weather and forecasts and then loads new data."""
+        if (self._location):
             self.clear_forecasts()
-            self.find_forecast(self.__location)
+            self.find_forecast(self._location)