Merge lp:~kai-mast/friends/fetch-twitter-images into lp:friends
- fetch-twitter-images
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 245 |
Proposed branch: | lp:~kai-mast/friends/fetch-twitter-images |
Merge into: | lp:friends |
Diff against target: |
127 lines (+38/-21) 3 files modified
friends/protocols/twitter.py (+19/-5) friends/tests/data/twitter-home.dat (+13/-12) friends/tests/test_twitter.py (+6/-4) |
To merge this branch: | bzr merge lp:~kai-mast/friends/fetch-twitter-images |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Robert Bruce Park | Approve | ||
Kai Mast (community) | Needs Resubmitting | ||
Review via email: mp+197556@code.launchpad.net |
Commit message
Description of the change
Maybe this was too easy and I did something wrong. But for me this small change resulted in twitter posts now properly showing images.
Kai Mast (kai-mast) wrote : | # |
Done :)
Robert Bruce Park (robru) wrote : | # |
Please specify a default value for picture because python's None value i don't think will work. So:
picture = url.get(
Oh and please add a test for this as well (look at the existing tests, you might have to add some extra json if there isn't already a media_url key present in the example data).
Kai Mast (kai-mast) wrote : | # |
Tests added. The diff got a lot larger now because I changed the code to only adding the media_url if it exists (otherwise the tests break).
Kai Mast (kai-mast) wrote : | # |
Btw, considering the second TODO I added. I was wondering, if I can just add a html tag at the protocol level? Or will this break internal processing?
Robert Bruce Park (robru) wrote : | # |
What? No, I don't like your args dict creation at all. Put back the self._publish call the way it was, and just update the tests to reflect the new argument being called.
Also I'm not sure what you mean by "adding an html tag at the protocol level". The url field can only be one URL, because that's how friends-app interprets it. Putting HTML in there will break how friends-app displays it.
Robert Bruce Park (robru) wrote : | # |
Also, why did you review as 'Resubmit'? Do you intend to create a new MP?
Adolfo Jayme Barrientos (fitojb) wrote : | # |
> Also, why did you review as 'Resubmit'? Do you intend to create a new MP?
If Launchpad was less confusing... When I created my first MPs, I thought "Resubmit" was a way to indicate that a change was made to the MP - MP reviews display similarly to bug statuses in LP, so one can think they work the same way. </sorry_
Robert Bruce Park (robru) wrote : | # |
Adolfo, are there any launchpad projects you don't subscribe to? ;-)
Kai Mast (kai-mast) wrote : | # |
@Adolfo: yes that is exactly what I thought
Robert, if I don't use the args dictionary I woudl have to add link_picture='' to some of the tests. That was in my eyes a worse than adding the dictionary, but if you prefer that I will do so.
What I mean with the other post was the following: The t.co link is currently replaced with the long link instead of the pretty display link. The easiest way to solve this would be to directly insert an html tag at line 37.
But there seems to be some processing at a higer level that replaces links by HTML tags, so I don't know it if this would break in such a case...
Kai Mast (kai-mast) wrote : | # |
I just tested the latter on my branch and it seems to work :)
So we would have:
<a href='http://
Instead of:
<a href="http://
Robert Bruce Park (robru) wrote : | # |
Oh right, it's starting to come back to me now, we do have some code in the linkifier regex that stops it linkifying links that are already linkified, so that just might work.
yes, please change back the args dict / _publish call and update all the tests to expect "link_picture=''". I find a dozen lines of "args args args args args args args" with the accompanying bracket notation really ugly. Strongly prefer direct kwargs on the _publish method invocation.
Kai Mast (kai-mast) wrote : | # |
Args dictionary removed...
Robert Bruce Park (robru) wrote : | # |
Ok, this is starting to look really good to me now ;-)
Please:
1. Strip trailing whitespace
2. Add trailing comma to last argument of self._publish() (adding it now means the diff won't be ugly later if we have to add another argument at the end)
3. "picture = ''; if 'media_url' in url: picture = url.get(
4. "if display_url is not ''" is really ugly. At the very least, it should just be "if display_url:", however I don't like the two different "message = message[...]" lines, I think we can do better than this. Please try something like this instead:
expanded_url = url.get(
display_url = url.get(
other_url = url.get('url', '')
picture_url = url.get(
if None not in (begin, end):
message = ''.join(
'<a href="',
'">',
'</a>',
This is a much more concise/pythonic way of expressing the same thought, and it's also a little bit more robust in the case of missing keys from the url dict.
Kai Mast (kai-mast) wrote : | # |
I hink I did what you said.
Its my first time touching reallife python code so I have no idea of coding style. Thanks for the feedback.
Robert Bruce Park (robru) wrote : | # |
Ok, yes it looks very good to me now. I see you caught a bug I wrote, since I forgot to put the arguments to 'join' in a list ;-)
I will try to test this a bit more thoroughly later today. Thanks!
Robert Bruce Park (robru) wrote : | # |
Ok, gave this a proper test, it looks really good! Thanks!
Preview Diff
1 | === modified file 'friends/protocols/twitter.py' |
2 | --- friends/protocols/twitter.py 2013-07-23 08:17:16 +0000 |
3 | +++ friends/protocols/twitter.py 2013-12-05 19:59:40 +0000 |
4 | @@ -119,16 +119,29 @@ |
5 | tweet_id=tweet_id) |
6 | |
7 | message = tweet.get('text', '') |
8 | + picture_url = '' |
9 | |
10 | # Resolve t.co links. |
11 | + #TODO support more than one media file |
12 | entities = tweet.get('entities', {}) |
13 | for url in (entities.get('urls', []) + entities.get('media', [])): |
14 | begin, end = url.get('indices', (None, None)) |
15 | - destination = (url.get('expanded_url') or |
16 | - url.get('display_url') or |
17 | - url.get('url')) |
18 | - if None not in (begin, end, destination): |
19 | - message = message[:begin] + destination + message[end:] |
20 | + |
21 | + expanded_url = url.get('expanded_url', '') |
22 | + display_url = url.get('display_url', '') |
23 | + other_url = url.get('url', '') |
24 | + picture_url = url.get('media_url', '') |
25 | + |
26 | + # Friends has no notion of display URLs, so this is handled at the protocol level |
27 | + if None not in (begin, end): |
28 | + message = ''.join([ |
29 | + message[:begin], |
30 | + '<a href="', |
31 | + (expanded_url or display_url or other_url), |
32 | + '">', |
33 | + (display_url or expanded_url or other_url), |
34 | + '</a>', |
35 | + message[end:]]) |
36 | |
37 | self._publish( |
38 | message_id=tweet_id, |
39 | @@ -142,6 +155,7 @@ |
40 | icon_uri=avatar_url.replace('_normal.', '.'), |
41 | liked=tweet.get('favorited', False), |
42 | url=permalink, |
43 | + link_picture=picture_url, |
44 | ) |
45 | return permalink |
46 | |
47 | |
48 | === modified file 'friends/tests/data/twitter-home.dat' |
49 | --- friends/tests/data/twitter-home.dat 2012-10-13 01:27:15 +0000 |
50 | +++ friends/tests/data/twitter-home.dat 2013-12-05 19:59:40 +0000 |
51 | @@ -100,17 +100,18 @@ |
52 | "id_str": "240556426106372096", |
53 | "in_reply_to_user_id_str": null, |
54 | "entities": { |
55 | - "urls": [ |
56 | - { |
57 | - "expanded_url": "http://blogs.ischool.berkeley.edu/i290-abdt-s12/", |
58 | - "url": "http://t.co/bfj7zkDJ", |
59 | - "indices": [ |
60 | - 79, |
61 | - 99 |
62 | - ], |
63 | - "display_url": "blogs.ischool.berkeley.edu/i290-abdt-s12/" |
64 | - } |
65 | - ], |
66 | + "media":[{"type":"photo", |
67 | + "sizes":{ |
68 | + "thumb":{"h":150, "resize":"crop", "w":150}, |
69 | + "large":{"h":238, "resize":"fit", "w":226}, |
70 | + "medium":{"h":238, "resize":"fit", "w":226}, |
71 | + "small":{"h":238, "resize":"fit", "w":226}}, |
72 | + "indices":[78,98], "url":"http://t.co/rJC5Pxsu", |
73 | + "media_url":"http://p.twimg.com/AZVLmp-CIAAbkyy.jpg", |
74 | + "display_url":"pic.twitter.com\/rJC5Pxsu", |
75 | + "id":114080493040967680, "id_str":"114080493040967680", |
76 | + "expanded_url": "http://twitter.com/yunorno/status/114080493036773378/photo/1", |
77 | + "media_url_https":"https://p.twimg.com/AZVLmp-CIAAbkyy.jpg"}], |
78 | "hashtags": [ |
79 | |
80 | ], |
81 | @@ -137,7 +138,7 @@ |
82 | } |
83 | ] |
84 | }, |
85 | - "text": "lecturing at the \"analyzing big data with twitter\" class at @cal with @othman http://t.co/bfj7zkDJ", |
86 | + "text": "lecturing at the \"analyzing big data with twitter\" class at @cal with @othman http://t.co/rJC5Pxsu", |
87 | "contributors": null, |
88 | "id": 240556426106372096, |
89 | "retweet_count": 3, |
90 | |
91 | === modified file 'friends/tests/test_twitter.py' |
92 | --- friends/tests/test_twitter.py 2013-11-01 17:27:51 +0000 |
93 | +++ friends/tests/test_twitter.py 2013-12-05 19:59:40 +0000 |
94 | @@ -133,6 +133,7 @@ |
95 | self.assertEqual(3, TestModel.get_n_rows()) |
96 | |
97 | # This test data was ripped directly from Twitter's API docs. |
98 | + |
99 | expected = [ |
100 | ['twitter', 88, '240558470661799936', |
101 | 'messages', 'OAuth Dancer', '119476949', 'oauth_dancer', False, |
102 | @@ -144,13 +145,13 @@ |
103 | ['twitter', 88, '240556426106372096', |
104 | 'messages', 'Raffi Krikorian', '8285392', 'raffi', False, |
105 | '2012-08-28T21:08:15Z', 'lecturing at the "analyzing big data ' |
106 | - 'with twitter" class at @cal with @othman ' |
107 | - '<a href="http://blogs.ischool.berkeley.edu/i290-abdt-s12/">' |
108 | - 'http://blogs.ischool.berkeley.edu/i290-abdt-s12/</a>', |
109 | + 'with twitter" class at @cal with @othman ' |
110 | + '<a href="http://twitter.com/yunorno/status/114080493036773378/photo/1">' |
111 | + 'pic.twitter.com/rJC5Pxsu</a>', |
112 | 'https://si0.twimg.com/profile_images/1270234259/' |
113 | 'raffi-headshot-casual.png', |
114 | 'https://twitter.com/raffi/status/240556426106372096', |
115 | - 0, False, '', '', '', '', '', '', '', 0.0, 0.0, |
116 | + 0, False, 'http://p.twimg.com/AZVLmp-CIAAbkyy.jpg', '', '', '', '', '', '', 0.0, 0.0, |
117 | ], |
118 | ['twitter', 88, '240539141056638977', |
119 | 'messages', 'Taylor Singletary', '819797', 'episod', False, |
120 | @@ -313,6 +314,7 @@ |
121 | liked=False, sender='Bob', stream='private', |
122 | url='https://twitter.com/some_guy/status/1452456', |
123 | icon_uri='https://example.com/bob.jpg', |
124 | + link_picture='', |
125 | sender_nick='some_guy', sender_id='', from_me=False, |
126 | timestamp='2012-11-04T17:14:52Z', message='Does my avatar show up?', |
127 | message_id='1452456') |
Please bring back the trailing comma at the end of url=permalink. It will make the diff nicer.