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.

Sign up for GitHub

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

Jump to bottom

(Tumblr) Add nil check to prevent undefined method error (fixes #29) #179

Merged
merged 4 commits into from
May 7, 2015
Merged

(Tumblr) Add nil check to prevent undefined method error (fixes #29) #179

merged 4 commits into from
May 7, 2015

Conversation

paramaggarwal
Copy link
Contributor

Fix for this error when importing a Tumblr blog with video posts with empty video-player field:

Fetching http://paramaggarwal.com/api/read/json/?num=50&start=0
Page: 1 - Posts: 50
/usr/local/lib/ruby/gems/2.2.0/gems/jekyll-import-0.5.3/lib/jekyll-import/importers/tumblr.rb:136:in `post_to_hash': undefined method `<<' for false:FalseClass (NoMethodError)
    from /usr/local/lib/ruby/gems/2.2.0/gems/jekyll-import-0.5.3/lib/jekyll-import/importers/tumblr.rb:47:in `block in process'
    from /usr/local/lib/ruby/gems/2.2.0/gems/jekyll-import-0.5.3/lib/jekyll-import/importers/tumblr.rb:47:in `map'
    from /usr/local/lib/ruby/gems/2.2.0/gems/jekyll-import-0.5.3/lib/jekyll-import/importers/tumblr.rb:47:in `process'
    from /usr/local/lib/ruby/gems/2.2.0/gems/jekyll-import-0.5.3/lib/jekyll-import/importer.rb:23:in `run'
    from -e:2:in `<main>'

@paramaggarwal paramaggarwal mentioned this pull request Jan 8, 2015
Closed
@parkr
Copy link
Member

parkr commented Jan 10, 2015

I don't think this will fix the error you got. You got an error because content was false.

@paramaggarwal
Copy link
Contributor Author

Yes, you are right! I ran the importer after this change and it worked so I thought this fixed it. Now I'm confused.

parkr
parkr reviewed Jan 10, 2015
View reviewed changes
lib/jekyll-import/importers/tumblr.rb
@@ -131,8 +131,10 @@ def self.post_to_hash(post, format)
when "video"
title = post["video-title"]
content = post["video-player"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If content is false, then it means something went wrong here.

@parkr
Copy link
Member

parkr commented Jan 10, 2015

it worked so I thought this fixed it.

It could have been that it also had no video-caption, so it never called #<< on content.

@octalmage
Copy link

This fixed the errors I was getting:

/Users/jasonstallings/.rvm/rubies/ruby-2.1.4/lib/ruby/gems/2.1.0/gems/jekyll-import-0.6.0/lib/jekyll-import/importers/tumblr.rb:135:in `post_to_hash': undefined method `<<' for false:FalseClass (NoMethodError)
        from /Users/jasonstallings/.rvm/rubies/ruby-2.1.4/lib/ruby/gems/2.1.0/gems/jekyll-import-0.6.0/lib/jekyll-import/importers/tumblr.rb:47:in `block in process'
        from /Users/jasonstallings/.rvm/rubies/ruby-2.1.4/lib/ruby/gems/2.1.0/gems/jekyll-import-0.6.0/lib/jekyll-import/importers/tumblr.rb:47:in `map'
        from /Users/jasonstallings/.rvm/rubies/ruby-2.1.4/lib/ruby/gems/2.1.0/gems/jekyll-import-0.6.0/lib/jekyll-import/importers/tumblr.rb:47:in `process'
        from /Users/jasonstallings/.rvm/rubies/ruby-2.1.4/lib/ruby/gems/2.1.0/gems/jekyll-import-0.6.0/lib/jekyll-import/importer.rb:23:in `run'
        from -e:2:in `<main>'

Import process worked without error with this change.

parkr
parkr reviewed Mar 31, 2015
View reviewed changes
lib/jekyll-import/importers/tumblr.rb
@@ -131,8 +131,10 @@ def self.post_to_hash(post, format)
when "video"
title = post["video-title"]
content = post["video-player"]
unless post["video-caption"].nil?
content << "<br/>" + post["video-caption"]
unless post[:content].nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this check content?

unless content.nil? || post["video-caption"].nil?
  content << "<br/>" + post["video-caption"]
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, would you like me to make this change to the PR? Unfortunately, I would have no way to try it out though...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please! :)

@paramaggarwal
Copy link
Contributor Author

@parkr Updated the PR with your review comments. Sorry for the immense delay for making this change...

parkr
parkr reviewed May 6, 2015
View reviewed changes
lib/jekyll-import/importers/tumblr.rb
@@ -131,7 +131,7 @@ def self.post_to_hash(post, format)
when "video"
title = post["video-title"]
content = post["video-player"]
unless post["video-caption"].nil?
unless content.nil? || post["video-caption"].nil?
content << "<br/>" + post["video-caption"]
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if content is nil, should we say content = post["video-caption"]?

@paramaggarwal
Copy link
Contributor Author

@parkr Makes sense! Updated PR.

@parkr
Copy link
Member

parkr commented May 7, 2015

🎉

parkr added a commit that referenced this pull request May 7, 2015
@parkr
Merge pull request #179 from paramaggarwal/patch-1
fc3c1ff 
Merge pull request 179
@parkr parkr merged commit fc3c1ff into jekyll:master May 7, 2015
@paramaggarwal paramaggarwal deleted the patch-1 branch May 7, 2015 05:55
parkr added a commit that referenced this pull request May 7, 2015
@parkr
Update history to reflect merge of #179 [ci skip]
f87a404
@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
frozen-due-to-age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@paramaggarwal @parkr @octalmage @jekyllbot