Skip to content

Fix reddit top image - #29

Merged
codelucas merged 1 commit into
codelucas:masterfrom
otemnov:original
Jan 27, 2014
Merged

Fix reddit top image#29
codelucas merged 1 commit into
codelucas:masterfrom
otemnov:original

Conversation

@otemnov

@otemnov otemnov commented Jan 27, 2014

Copy link
Copy Markdown
Contributor

Article.py set_reddit_top_img never works as self.top_img is not empty string in any case.

 def set_reddit_top_img(self):
        if self.top_img != u'': # if we already have a top img...
            return
        # select image with top size

Reason is simple set_top_img could encode None to "None" string:

  def set_top_img(self, src_url):
        src_url = encodeValue(src_url)
        self.top_img = src_url
        self.top_image = src_url

@codelucas

Copy link
Copy Markdown
Owner

You are right, thank you for finding this subtle bug! Before merging it, don't you think a better fix would be having our check be if self.top_img: return versus if self.top_img != u'': return?

I think we should just change the clause to be if top_img: instead of if self.top_img != u''. Or, maybe at least change the default return value in https://github.com/codelucas/newspaper/blob/master/newspaper/parsers.py#L98 to be u"" instead of None?

Feel free to send in another PR and i'll most likely merge. I don't know if making such a big change in the encode method is such a good idea.

@codelucas

Copy link
Copy Markdown
Owner

I changed my mind, I like your fix more. Merging now.

codelucas added a commit that referenced this pull request Jan 27, 2014
@codelucas
codelucas merged commit adba1d0 into codelucas:master Jan 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants