Merge lp:~kai-mast/friends/fix-retweets into lp:friends

[Kai Mast]

This branch fixes the truncation of retweets. It also allows to properly support RTs in the future.

Be free to correct my coding style ;)

[lp:~kai-mast/friends/fix-retweets]

262. By Kai Mast on 2013-12-06

Tried to removed whitespace in diff

[Robert Bruce Park (robru)]

So this mostly looks good, but there's a bad idiom you used here that you also did in your previous MP:

58 + if "retweeted_status" in tweet:
59 + shared_status = tweet.get('retweeted_status')

There's just no need for this kind of "look before you leap" logic in python. At worst, it's a race condition, and at best it's inefficient.

What you really want to do is something more like this:

message = tweet.get('retweeted_status', {}).get('text', '') or tweet.get('text', '')

If you do that, then you also reduce the number of calls to _resolve_tco() to 1, meaning you can inline it (meaning it doesn't have to be it's own method anymore).

Also you have "entities = tweet.get('entities', {})" in two different places, needlessly redundant.

review: Needs Fixing

[Kai Mast (kai-mast)]

But how would I add "RT"-part in this case? (see line 61 and 80)

[Robert Bruce Park (robru)]

Still better to just have "shared_status = tweet.get('retweeted_status', {}).get('text', '')" and then you can say "if shared_status: message = 'RT ' + message" or something. Testing a dict before pulling a value from a dict is really bad form in python.

[Kai Mast (kai-mast)]

Okay. I will do that.

Do you want me to inline the other stuff again? I really like small functions; they make the code more readable in my opinion.

[Robert Bruce Park (robru)]

Not sure right now... I like small functions too but I dislike functions
that only get called once :)

[lp:~kai-mast/friends/fix-retweets]

263. By Kai Mast on 2013-12-09

Simplified code as suggested

264. By Kai Mast on 2013-12-09

Diff cleanups

[Kai Mast (kai-mast)]

Okay the diff really is a lot smaller now.

I added an if-clause to fetching the media url. I think otherwise me may run into problems when the picture is not the last entity.

[Kai Mast (kai-mast)]

we*

[Robert Bruce Park (robru)]

Ok, this is looking good now just from reading it. I need a bit more time to review it fully, hopefully later today.

[Robert Bruce Park (robru)]

Kai, thanks for this branch. I merged it with some changes, you should review them here:

http://bazaar.launchpad.net/~super-friends/friends/trunk/revision/247

Note how I managed to achieve the same logic without having to do any dictionary membership tests.

review: Approve

[Kai Mast (kai-mast)]

Okay that really looks nicer. Thanks for merging.

Just wondering, how often is the friends package for trusty rebuilt? Would be nice to benefit from those changes.

[Robert Bruce Park (robru)]

It gets rebuilt every 4 hours, except that it doesn't, because of reasons.

So basically you have to ask for it. Right now there's an informal 'freeze' on releases because we're trying to get our automated phone-image testing system at 100% pass rate and we don't want to release *anything* that might inadvertently cause regressions. Over the last couple weeks we've gotten it from something like 80% to 98%, so it should be done soon. Hopefully we might be able to do a friends release this week, barring any catastrophic failures.

[Robert Bruce Park (robru)]

Kai, if you really need to test a newly built package, you can enable ppa:ubuntu-unity/daily-build and install friends from there (but be sure to disable the ppa before upgrading your system, most of the other stuff in that PPA will hose your system).

[Kai Mast (kai-mast)]

Okay cool. I just took the friends package from the PPA so nothing else should break.

As far as I see friends is not running in an upstart usersession yet. Is that also something I could tackle?

[Robert Bruce Park (robru)]

You're welcome to try! I haven't been keeping up with that stuff much myself.

=== modified file 'friends/protocols/twitter.py'
--- friends/protocols/twitter.py	2013-12-05 19:59:31 +0000
+++ friends/protocols/twitter.py	2013-12-09 19:56:55 +0000
@@ -117,20 +117,22 @@
         permalink = self._tweet_permalink.format(
             user_id=screen_name,
             tweet_id=tweet_id)
-
-        message = tweet.get('text', '')
+            
+        entities = tweet.get('retweeted_status', {}).get('entities', '') or tweet.get('entities', {})
+        message = tweet.get('retweeted_status', {}).get('text', '') or tweet.get('text', '')
         picture_url = ''
-
-        # Resolve t.co links.
-        #TODO support more than one media file
-        entities = tweet.get('entities', {})
+        
+        #Resolve t.co
+        #TODO support more than one url and/or media file
         for url in (entities.get('urls', []) + entities.get('media', [])):
             begin, end = url.get('indices', (None, None))
             
             expanded_url = url.get('expanded_url', '')
             display_url = url.get('display_url', '')
             other_url = url.get('url', '')
-            picture_url = url.get('media_url', '')
+            
+            if 'media_url' in url:
+                picture_url = url.get('media_url')
 
             # Friends has no notion of display URLs, so this is handled at the protocol level
             if None not in (begin, end):
@@ -143,6 +145,11 @@
                     '</a>',
                     message[end:]])
 
+        #Friends has no native support for retweets so we just encode it in the message 
+        if "retweeted_status" in tweet:
+            other_user = tweet.get('retweeted_status').get('user', {})
+            message = "RT @" + other_user.get('screen_name', '')  + ": " + message
+    
         self._publish(
             message_id=tweet_id,
             message=message,

=== modified file 'friends/tests/test_twitter.py'
--- friends/tests/test_twitter.py	2013-12-05 12:26:21 +0000
+++ friends/tests/test_twitter.py	2013-12-09 19:56:55 +0000
@@ -470,7 +470,7 @@
             'messages', 'Robert Bruce', '836242932', 'therealrobru', True,
             '2013-04-16T17:58:26Z', 'RT @tarek_ziade: Just found a "Notification '
             'of Inspection" card in the bottom of my bag. looks like they were '
-            'curious about those raspbe ...',
+            'curious about those raspberry-pi :O',
             'https://si0.twimg.com/profile_images/2631306428/'
             '2a509db8a05b4310394b832d34a137a4.png',
             'https://twitter.com/therealrobru/status/324220250889543682',