Code review comment for lp:~kai-mast/friends/fetch-twitter-images

Revision history for this message
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('media_url')" should just be "picture = url.get('media_url', '')"

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('expanded_url', '')
display_url = url.get('display_url', '')
other_url = url.get('url', '')
picture_url = url.get('media_url', '')

if None not in (begin, end):
    message = ''.join(
        message[:begin],
        '<a href="',
        (expanded_url or display_url or other_url),
        '">',
        (display_url or expanded_url or other_url),
        '</a>',
        message[end:])

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.

review: Needs Fixing

« Back to merge proposal