Friends

Merge lp:~kai-mast/friends/links into lp:friends

  1. links
  2. Merge into trunk
Proposed by Kai Mast
Status: Merged
Merged at revision: 249
Proposed branch: lp:~kai-mast/friends/links
Merge into: lp:friends
Diff against target: 437 lines (+361/-17)
4 files modified
friends/protocols/twitter.py (+48/-15)
friends/tests/data/twitter-hashtags.dat (+94/-0)
friends/tests/data/twitter-multiple-links.dat (+144/-0)
friends/tests/test_twitter.py (+75/-2)
To merge this branch: bzr merge lp:~kai-mast/friends/links
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Needs Fixing
Robert Bruce Park Approve
Review via email: mp+202954@code.launchpad.net

Description of the change

This branch likifies mentions and hashtags. Hashtags open a link with the search in a browser. Friends has support for searches but afaik there is no way to add links to them. So this is the best we can do right now. Imo that makes tweets (especially retweets) much more interakctive.

I also fixed a problem when there are multiple links in a tweet and also added a test so this stays fixed.

To post a comment you must log in.
Revision history for this message
Robert Bruce Park (robru) wrote :

Wow, congrats on the test coverage, that's really impressive ;-)

One thing that I'm a little bit confused about. You named a variable "urls_sorted" but I don't actually see any sorting going on there. Then you go on to use a concept of 'offset' in order to keep track of where each link should go, and I find it a bit sloppy.

What I'd like to see instead, is just iterate over urls_sorted in a sorted way, with descending values of 'begin' (eg, linkify the last URL first and progress backwards through the string). That way there's never an offset to have to worry about as the change in string length does not affect links that appear prior to the change being made.

Another (small) problem is that you used iter() in a useless way. Friends is python3-only, so dict.items() always returns an iter and your extra call to iter() doesn't do anything.

So instead it should probably look like this:

    for key, url in sorted(urls_sorted.items(), reverse=True):

And then...

    if content:
        message = ''.join([message[:begin], content, message[end:]])

Also, I'm slightly annoyed by the inconsistency between the _linkify_mention function which is nearly identical to other linkification techniques that are inlined in the larger function. I'm sure there can be a general purpose _linkify method that can be used to linkify all kinds of links, in a much more consistent and space-efficient manner (ie, fewer lines of code).

review: Needs Fixing
Revision history for this message
Kai Mast (kai-mast) wrote :

Oh okay I thought dictionaries where sorted by default (I am so used to C++ ;) )

I haven't even thought of going through the message in reverse to keep track off the offset. Good point.

Revision history for this message
Kai Mast (kai-mast) wrote :

Better?

I am not sure how to use just one linkify function because some have absolute addresses and some don't, so I now have linkify and linkify_mention, where the latter calls the first.

I also made the links use https which should be used whenever possible in my opinion.

Revision history for this message
Robert Bruce Park (robru) wrote :

This is a ton better, thank you!

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Autolanding.
No commit message was specified in the merge proposal. Hit 'Add commit message' on the merge proposal web page or follow the link below. You can approve the merge proposal yourself to rerun.
https://code.launchpad.net/~kai-mast/friends/links/+merge/202954/+edit-commit-message

review: Needs Fixing (continuous-integration)
Revision history for this message
Robert Bruce Park (robru) wrote :

One thing to watch out for is that you have a ton of trailing whitespace all over. I don't like to see that in my code ;-)

Don't worry, I'm cleaning it up for this merge, but if you could start stripping trailing whitespace before committing I'd appreciate it.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'friends/protocols/twitter.py'
2--- friends/protocols/twitter.py 2013-12-10 07:04:05 +0000
3+++ friends/protocols/twitter.py 2014-01-24 10:56:16 +0000
4@@ -127,29 +127,51 @@
5
6 #Resolve t.co
7 #TODO support more than one url and/or media file
8- for url in (entities.get('urls', []) + entities.get('media', [])):
9- begin, end = url.get('indices', (None, None))
10-
11- expanded_url = url.get('expanded_url', '')
12- display_url = url.get('display_url', '')
13- other_url = url.get('url', '')
14+ urls = {}
15+
16+ for url in (entities.get('urls', []) + entities.get('media', []) + entities.get('user_mentions', []) + entities.get('hashtags', [])):
17+ begin, end = url.get('indices', (None, None))
18+
19+ #Drop invalid entities (just to be safe)
20+ if None not in (begin, end):
21+ urls[begin] = url
22+
23+ for key, url in sorted(urls.items(), reverse = True):
24+ begin, end = url.get('indices', (None, None))
25+
26+ expanded_url = url.get('expanded_url')
27+ display_url = url.get('display_url')
28+ other_url = url.get('url')
29+
30+ mention_name = url.get('screen_name')
31
32 picture_url = url.get('media_url', picture_url)
33+
34+ hashtag = url.get('text')
35+
36+ content = None
37
38 # Friends has no notion of display URLs, so this is handled at the protocol level
39- if None not in (begin, end):
40+ if (other_url or expanded_url):
41+ content = self._linkify(expanded_url or other_url, display_url or other_url)
42+
43+ # Linkify hashtags until supported by friends
44+ if hashtag:
45+ content = self._linkify('https://twitter.com/search?q=%23' + hashtag + '&src=hash', '#' + hashtag)
46+
47+ # Linkify a mention until they are supported natively by friends
48+ if mention_name:
49+ content = self._linkify_mention(mention_name)
50+
51+ if content:
52 message = ''.join([
53 message[:begin],
54- '<a href="',
55- (expanded_url or other_url),
56- '">',
57- (display_url or other_url),
58- '</a>',
59+ content,
60 message[end:]])
61-
62+
63 if retweet:
64- message = 'RT @{}: {}'.format(
65- retweet.get('user', {}).get('screen_name', ''),
66+ message = 'RT {}: {}'.format(
67+ self._linkify_mention(retweet.get('user', {}).get('screen_name', '')),
68 message
69 )
70
71@@ -168,6 +190,17 @@
72 link_picture=picture_url,
73 )
74 return permalink
75+
76+ def _linkify_mention(self, name):
77+ return self._linkify('https://twitter.com/' + name, '@' + name)
78+
79+ def _linkify(self, address, name):
80+ return ''.join([
81+ '<a href="',
82+ address,
83+ '">',
84+ name,
85+ '</a>'])
86
87 def _append_since(self, url, stream='messages'):
88 since = self._tweet_ids.get(stream)
89
90=== added file 'friends/tests/data/twitter-hashtags.dat'
91--- friends/tests/data/twitter-hashtags.dat 1970-01-01 00:00:00 +0000
92+++ friends/tests/data/twitter-hashtags.dat 2014-01-24 10:56:16 +0000
93@@ -0,0 +1,94 @@
94+{
95+ "created_at": "Fri Jan 17 14:23:41 +0000 2014",
96+ "id": 424185261375766500,
97+ "id_str": "424185261375766530",
98+ "text": "A service that filters food pictures from Instagram. #MillionDollarIdea",
99+ "source": "web",
100+ "truncated": false,
101+ "in_reply_to_status_id": null,
102+ "in_reply_to_status_id_str": null,
103+ "in_reply_to_user_id": null,
104+ "in_reply_to_user_id_str": null,
105+ "in_reply_to_screen_name": null,
106+ "user": {
107+ "id": 17339829,
108+ "id_str": "17339829",
109+ "name": "Kai Mast",
110+ "screen_name": "Kai_Mast",
111+ "location": "Bamberg",
112+ "description": "Computer Science Student, Grad School Applicant, C++ Fanboy",
113+ "url": "http://t.co/1myW31Mlhl",
114+ "entities": {
115+ "url": {
116+ "urls": [
117+ {
118+ "url": "http://t.co/1myW31Mlhl",
119+ "expanded_url": "http://kai-mast.de",
120+ "display_url": "kai-mast.de",
121+ "indices": [
122+ 0,
123+ 22
124+ ]
125+ }
126+ ]
127+ },
128+ "description": {
129+ "urls": []
130+ }
131+ },
132+ "protected": false,
133+ "followers_count": 415,
134+ "friends_count": 904,
135+ "listed_count": 36,
136+ "created_at": "Wed Nov 12 14:23:09 +0000 2008",
137+ "favourites_count": 939,
138+ "utc_offset": 3600,
139+ "time_zone": "Berlin",
140+ "geo_enabled": true,
141+ "verified": false,
142+ "statuses_count": 7886,
143+ "lang": "en",
144+ "contributors_enabled": false,
145+ "is_translator": false,
146+ "profile_background_color": "8B542B",
147+ "profile_background_image_url": "http://abs.twimg.com/images/themes/theme8/bg.gif",
148+ "profile_background_image_url_https": "https://abs.twimg.com/images/themes/theme8/bg.gif",
149+ "profile_background_tile": false,
150+ "profile_image_url": "http://pbs.twimg.com/profile_images/424181800701673473/Q6Ggqg7P_normal.png",
151+ "profile_image_url_https": "https://pbs.twimg.com/profile_images/424181800701673473/Q6Ggqg7P_normal.png",
152+ "profile_banner_url": "https://pbs.twimg.com/profile_banners/17339829/1360006443",
153+ "profile_link_color": "9D582E",
154+ "profile_sidebar_border_color": "D9B17E",
155+ "profile_sidebar_fill_color": "EADEAA",
156+ "profile_text_color": "333333",
157+ "profile_use_background_image": true,
158+ "default_profile": false,
159+ "default_profile_image": false,
160+ "following": false,
161+ "follow_request_sent": false,
162+ "notifications": false
163+ },
164+ "geo": null,
165+ "coordinates": null,
166+ "place": null,
167+ "contributors": null,
168+ "retweet_count": 0,
169+ "favorite_count": 0,
170+ "entities": {
171+ "hashtags": [
172+ {
173+ "text": "MillionDollarIdea",
174+ "indices": [
175+ 53,
176+ 71
177+ ]
178+ }
179+ ],
180+ "symbols": [],
181+ "urls": [],
182+ "user_mentions": []
183+ },
184+ "favorited": false,
185+ "retweeted": false,
186+ "lang": "en"
187+}
188
189=== added file 'friends/tests/data/twitter-multiple-links.dat'
190--- friends/tests/data/twitter-multiple-links.dat 1970-01-01 00:00:00 +0000
191+++ friends/tests/data/twitter-multiple-links.dat 2014-01-24 10:56:16 +0000
192@@ -0,0 +1,144 @@
193+{
194+ "created_at": "Thu Jan 23 11:40:35 +0000 2014",
195+ "id": 426318539796930560,
196+ "id_str": "426318539796930560",
197+ "text": "An old people's home has recreated famous movie scenes for a wonderful calendar http://t.co/jjqteYzur0 http://t.co/JxQTPG7WLL",
198+ "source": "<a href=\"https://about.twitter.com/products/tweetdeck\" rel=\"nofollow\">TweetDeck</a>",
199+ "truncated": false,
200+ "in_reply_to_status_id": null,
201+ "in_reply_to_status_id_str": null,
202+ "in_reply_to_user_id": null,
203+ "in_reply_to_user_id_str": null,
204+ "in_reply_to_screen_name": null,
205+ "user": {
206+ "id": 16973333,
207+ "id_str": "16973333",
208+ "name": "The Independent",
209+ "screen_name": "Independent",
210+ "location": "London, United Kingdom",
211+ "description": "News, comment and features from The Independent. Also follow: @IndyVoices, \r\n@IndyPolitics, @IndyWorld and our journalists at http://t.co/YjS7NcXK4A",
212+ "url": "http://t.co/wDS5ly0QoO",
213+ "entities": {
214+ "url": {
215+ "urls": [
216+ {
217+ "url": "http://t.co/wDS5ly0QoO",
218+ "expanded_url": "http://www.independent.co.uk",
219+ "display_url": "independent.co.uk",
220+ "indices": [
221+ 0,
222+ 22
223+ ]
224+ }
225+ ]
226+ },
227+ "description": {
228+ "urls": [
229+ {
230+ "url": "http://t.co/YjS7NcXK4A",
231+ "expanded_url": "http://ind.pn/Wdlm9a",
232+ "display_url": "ind.pn/Wdlm9a",
233+ "indices": [
234+ 126,
235+ 148
236+ ]
237+ }
238+ ]
239+ }
240+ },
241+ "protected": false,
242+ "followers_count": 505759,
243+ "friends_count": 1747,
244+ "listed_count": 7890,
245+ "created_at": "Sun Oct 26 00:00:29 +0000 2008",
246+ "favourites_count": 29,
247+ "utc_offset": 0,
248+ "time_zone": "London",
249+ "geo_enabled": false,
250+ "verified": true,
251+ "statuses_count": 58003,
252+ "lang": "en",
253+ "contributors_enabled": false,
254+ "is_translator": false,
255+ "profile_background_color": "EBEBEB",
256+ "profile_background_image_url": "http://a0.twimg.com/profile_background_images/378800000119288704/4ac964c83462c88837dc1e735aa1a45e.png",
257+ "profile_background_image_url_https": "https://si0.twimg.com/profile_background_images/378800000119288704/4ac964c83462c88837dc1e735aa1a45e.png",
258+ "profile_background_tile": true,
259+ "profile_image_url": "http://pbs.twimg.com/profile_images/378800000706113664/d1a957578723e496c025be1e2577d06d_normal.jpeg",
260+ "profile_image_url_https": "https://pbs.twimg.com/profile_images/378800000706113664/d1a957578723e496c025be1e2577d06d_normal.jpeg",
261+ "profile_link_color": "FC051A",
262+ "profile_sidebar_border_color": "FFFFFF",
263+ "profile_sidebar_fill_color": "FFFFFF",
264+ "profile_text_color": "333333",
265+ "profile_use_background_image": true,
266+ "default_profile": false,
267+ "default_profile_image": false,
268+ "following": false,
269+ "follow_request_sent": false,
270+ "notifications": false
271+ },
272+ "geo": null,
273+ "coordinates": null,
274+ "place": null,
275+ "contributors": null,
276+ "retweet_count": 131,
277+ "favorite_count": 83,
278+ "entities": {
279+ "hashtags": [],
280+ "symbols": [],
281+ "urls": [
282+ {
283+ "url": "http://t.co/jjqteYzur0",
284+ "expanded_url": "http://ind.pn/1g3wX9q",
285+ "display_url": "ind.pn/1g3wX9q",
286+ "indices": [
287+ 80,
288+ 102
289+ ]
290+ }
291+ ],
292+ "user_mentions": [],
293+ "media": [
294+ {
295+ "id": 426318539692056600,
296+ "id_str": "426318539692056576",
297+ "indices": [
298+ 103,
299+ 125
300+ ],
301+ "media_url": "http://pbs.twimg.com/media/BeqWc_-CIAAhmdc.jpg",
302+ "media_url_https": "https://pbs.twimg.com/media/BeqWc_-CIAAhmdc.jpg",
303+ "url": "http://t.co/JxQTPG7WLL",
304+ "display_url": "pic.twitter.com/JxQTPG7WLL",
305+ "expanded_url": "http://twitter.com/Independent/status/426318539796930560/photo/1",
306+ "type": "photo",
307+ "sizes": {
308+ "thumb": {
309+ "w": 150,
310+ "h": 150,
311+ "resize": "crop"
312+ },
313+ "large": {
314+ "w": 1024,
315+ "h": 682,
316+ "resize": "fit"
317+ },
318+ "small": {
319+ "w": 340,
320+ "h": 226,
321+ "resize": "fit"
322+ },
323+ "medium": {
324+ "w": 600,
325+ "h": 400,
326+ "resize": "fit"
327+ }
328+ }
329+ }
330+ ]
331+ },
332+ "favorited": false,
333+ "retweeted": false,
334+ "possibly_sensitive": false,
335+ "lang": "en"
336+}
337
338=== modified file 'friends/tests/test_twitter.py'
339--- friends/tests/test_twitter.py 2013-12-05 20:45:19 +0000
340+++ friends/tests/test_twitter.py 2014-01-24 10:56:16 +0000
341@@ -145,7 +145,8 @@
342 ['twitter', 88, '240556426106372096',
343 'messages', 'Raffi Krikorian', '8285392', 'raffi', False,
344 '2012-08-28T21:08:15Z', 'lecturing at the "analyzing big data '
345- 'with twitter" class at @cal with @othman '
346+ 'with twitter" class at <a href="https://twitter.com/Cal">@Cal</a>'
347+ ' with <a href="https://twitter.com/othman">@othman</a> '
348 '<a href="http://twitter.com/yunorno/status/114080493036773378/photo/1">'
349 'pic.twitter.com/rJC5Pxsu</a>',
350 'https://si0.twimg.com/profile_images/1270234259/'
351@@ -468,7 +469,7 @@
352 expected_row = [
353 'twitter', 88, '324220250889543682',
354 'messages', 'Robert Bruce', '836242932', 'therealrobru', True,
355- '2013-04-16T17:58:26Z', 'RT @tarek_ziade: Just found a "Notification '
356+ '2013-04-16T17:58:26Z', 'RT <a href="https://twitter.com/tarek_ziade">@tarek_ziade</a>: Just found a "Notification '
357 'of Inspection" card in the bottom of my bag. looks like they were '
358 'curious about those raspberry-pi :O',
359 'https://si0.twimg.com/profile_images/2631306428/'
360@@ -478,6 +479,78 @@
361 ]
362 self.assertEqual(list(TestModel.get_row(0)), expected_row)
363
364+
365+ @mock.patch('friends.utils.base.Model', TestModel)
366+ @mock.patch('friends.utils.http.Soup.Message',
367+ FakeSoupMessage('friends.tests.data', 'twitter-multiple-links.dat'))
368+ @mock.patch('friends.protocols.twitter.Twitter._login',
369+ return_value=True)
370+ @mock.patch('friends.utils.base._seen_ids', {})
371+ def test_multiple_links(self, *mocks):
372+ self.account.access_token = 'access'
373+ self.account.secret_token = 'secret'
374+ self.account.user_name = 'Independent'
375+ self.account.auth.parameters = dict(
376+ ConsumerKey='key',
377+ ConsumerSecret='secret')
378+
379+ self.assertEqual(0, TestModel.get_n_rows())
380+ self.assertEqual(
381+ self.protocol.send('some message'),
382+ 'https://twitter.com/Independent/status/426318539796930560')
383+ self.assertEqual(1, TestModel.get_n_rows())
384+
385+ self.maxDiff = None
386+ expected_row = [
387+ 'twitter', 88, '426318539796930560',
388+ 'messages', 'The Independent', '16973333', 'Independent', True,
389+ '2014-01-23T11:40:35Z',
390+ 'An old people\'s home has recreated famous movie scenes for a wonderful calendar '
391+ '<a href="http://ind.pn/1g3wX9q">ind.pn/1g3wX9q</a> '
392+ '<a href="http://twitter.com/Independent/status/426318539796930560/photo/1">pic.twitter.com/JxQTPG7WLL</a>',
393+ 'https://pbs.twimg.com/profile_images/378800000706113664/d1a957578723e496c025be1e2577d06d.jpeg',
394+ 'https://twitter.com/Independent/status/426318539796930560',
395+ 0, False,
396+ 'http://pbs.twimg.com/media/BeqWc_-CIAAhmdc.jpg',
397+ '', '', '', '', '', '', 0.0, 0.0,
398+ ]
399+ self.assertEqual(list(TestModel.get_row(0)), expected_row)
400+
401+ @mock.patch('friends.utils.base.Model', TestModel)
402+ @mock.patch('friends.utils.http.Soup.Message',
403+ FakeSoupMessage('friends.tests.data', 'twitter-hashtags.dat'))
404+ @mock.patch('friends.protocols.twitter.Twitter._login',
405+ return_value=True)
406+ @mock.patch('friends.utils.base._seen_ids', {})
407+ def test_multiple_links(self, *mocks):
408+ self.account.access_token = 'access'
409+ self.account.secret_token = 'secret'
410+ self.account.user_name = 'Independent'
411+ self.account.auth.parameters = dict(
412+ ConsumerKey='key',
413+ ConsumerSecret='secret')
414+
415+ self.assertEqual(0, TestModel.get_n_rows())
416+ self.assertEqual(
417+ self.protocol.send('some message'),
418+ 'https://twitter.com/Kai_Mast/status/424185261375766530')
419+ self.assertEqual(1, TestModel.get_n_rows())
420+
421+ self.maxDiff = None
422+ expected_row = [
423+ 'twitter', 88, '424185261375766530',
424+ 'messages', 'Kai Mast', '17339829', 'Kai_Mast', False,
425+ '2014-01-17T14:23:41Z',
426+ 'A service that filters food pictures from Instagram. '
427+ '<a href="https://twitter.com/search?q=%23MillionDollarIdea&src=hash">#MillionDollarIdea</a>',
428+ 'https://pbs.twimg.com/profile_images/424181800701673473/Q6Ggqg7P.png',
429+ 'https://twitter.com/Kai_Mast/status/424185261375766530',
430+ 0, False,
431+ '',
432+ '', '', '', '', '', '', 0.0, 0.0,
433+ ]
434+ self.assertEqual(list(TestModel.get_row(0)), expected_row)
435+
436 def test_unfollow(self):
437 get_url = self.protocol._get_url = mock.Mock()
438

Subscribers

People subscribed via source and target branches

to all changes: