Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix reddit top image #29

Merged
merged 1 commit into from
Jan 27, 2014
Merged

Fix reddit top image #29

merged 1 commit into from
Jan 27, 2014

Conversation

otemnov
Copy link
Contributor

@otemnov otemnov commented Jan 27, 2014

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
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
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