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

Only complete write_post on Tumblr posts w/ non-nil content #118

Merged
merged 5 commits into from Jan 9, 2014

Conversation

Projects
None yet
4 participants
@vidbina
Contributor

vidbina commented Jan 1, 2014

Answers to questions on Tumblr appear as posts without content.
Whilst looping through the posts we are therefore to ignore all nil-content posts as JekyllImport::Importers::Tumblr#write_post fails when it tries adding nil to a string.

f.puts post[:header].to_yaml + "---\n" + nil

Perhaps we do want to add these posts in which case we'll at least have to ensure we don't attempt to append nil to the string by perhaps opting for

f.puts post[:header].to_yaml + "---\n" + (content or "")
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 1, 2014

Member

This looks like it just filters out the answer posts, if, for answer posts, post[:content] is nil. Would you mind adding in a test for this with a sample author post?

Member

parkr commented Jan 1, 2014

This looks like it just filters out the answer posts, if, for answer posts, post[:content] is nil. Would you mind adding in a test for this with a sample author post?

Show outdated Hide outdated jekyll-import.gemspec
@@ -34,7 +34,7 @@ Gem::Specification.new do |s|
# test dependencies:
s.add_development_dependency('redgreen', "~> 1.2")
s.add_development_dependency('shoulda', "~> 3.3.2")
s.add_development_dependency('shoulda', "~> 3.5.0")

This comment has been minimized.

@vidbina

vidbina Jan 3, 2014

Contributor

Kept bumping my head into warning: already initialized constant Mocha until I discovered that shoulda might be due for an update

@vidbina

vidbina Jan 3, 2014

Contributor

Kept bumping my head into warning: already initialized constant Mocha until I discovered that shoulda might be due for an update

end
@jsonPayload = '{"tumblelog":{"title":"JekyllImport","description":"Jekyll Importer Test.","name":"JekyllImport","timezone":"Canada\/Atlantic","cname":"https://github.com/jekyll/jekyll-import/","feeds":[]},"posts-start":0,"posts-total":"2","posts-type":false,"posts":[{"id":54759400073,"url":"https:\/\/github.com\/post\/54759400073","url-with-slug":"http:\/\/github.com\/post\/54759400073\/jekyll-test","type":"regular","date-gmt":"2013-07-06 16:27:23 GMT","date":"Sat, 06 Jul 2013 13:27:23","bookmarklet":null,"mobile":null,"feed-item":"","from-feed-id":0,"unix-timestamp":1373128043,"format":"html","reblog-key":"0L6yPcHr","slug":"jekyll-test","regular-title":"Jekyll: Test","regular-body":"<p>Testing...<\/p>","tags":["jekyll"]},{"id":"71845593082","url":"http:\/\/example.com\/post\/71845593082","url-with-slug":"http:\/\/example.com\/post\/71845593082\/knock-knock","type":"answer","date-gmt":"2014-01-01 14:08:45 GMT","date":"Wed, 01 Jan 2014 09:08:45","bookmarklet":0,"mobile":0,"feed-item":"","from-feed-id":0,"unix-timestamp":1388585325,"format":"html","reblog-key":"jPfWHFnT","slug":"knock-knock","question":"Knock knock?","answer":"<p>Who is there?<\/p>"}]}'
@posts = JSON.parse(@jsonPayload)
@batch = @posts["posts"].map { |post| Importers::Tumblr.post_to_hash(post, 'html') }

This comment has been minimized.

@vidbina

vidbina Jan 3, 2014

Contributor

Took the liberty to drop build the batch list in the setup as we're using it in nearly every test 😉, hope it's okay. I also dropped all the \t's in exhange for soft-tabs. Expect it to be accepted as I'm just following the coding style in the Ruby Styleguide which is referred to on Jekyll Contributing.

@vidbina

vidbina Jan 3, 2014

Contributor

Took the liberty to drop build the batch list in the setup as we're using it in nearly every test 😉, hope it's okay. I also dropped all the \t's in exhange for soft-tabs. Expect it to be accepted as I'm just following the coding style in the Ruby Styleguide which is referred to on Jekyll Contributing.

@jsonPayload = '{"tumblelog":{"title":"JekyllImport","description":"Jekyll Importer Test.","name":"JekyllImport","timezone":"Canada\/Atlantic","cname":"https://github.com/jekyll/jekyll-import/","feeds":[]},"posts-start":0,"posts-total":"1","posts-type":false,"posts":[{"id":54759400073,"url":"https:\/\/github.com\/post\/54759400073","url-with-slug":"http:\/\/github.com\/post\/54759400073\/jekyll-test","type":"regular","date-gmt":"2013-07-06 16:27:23 GMT","date":"Sat, 06 Jul 2013 13:27:23","bookmarklet":null,"mobile":null,"feed-item":"","from-feed-id":0,"unix-timestamp":1373128043,"format":"html","reblog-key":"0L6yPcHr","slug":"jekyll-test","regular-title":"Jekyll: Test","regular-body":"<p>Testing...<\/p>","tags":["jekyll"]}]}'
@posts = JSON.parse(@jsonPayload)
end
@jsonPayload = '{"tumblelog":{"title":"JekyllImport","description":"Jekyll Importer Test.","name":"JekyllImport","timezone":"Canada\/Atlantic","cname":"https://github.com/jekyll/jekyll-import/","feeds":[]},"posts-start":0,"posts-total":"2","posts-type":false,"posts":[{"id":54759400073,"url":"https:\/\/github.com\/post\/54759400073","url-with-slug":"http:\/\/github.com\/post\/54759400073\/jekyll-test","type":"regular","date-gmt":"2013-07-06 16:27:23 GMT","date":"Sat, 06 Jul 2013 13:27:23","bookmarklet":null,"mobile":null,"feed-item":"","from-feed-id":0,"unix-timestamp":1373128043,"format":"html","reblog-key":"0L6yPcHr","slug":"jekyll-test","regular-title":"Jekyll: Test","regular-body":"<p>Testing...<\/p>","tags":["jekyll"]},{"id":"71845593082","url":"http:\/\/example.com\/post\/71845593082","url-with-slug":"http:\/\/example.com\/post\/71845593082\/knock-knock","type":"answer","date-gmt":"2014-01-01 14:08:45 GMT","date":"Wed, 01 Jan 2014 09:08:45","bookmarklet":0,"mobile":0,"feed-item":"","from-feed-id":0,"unix-timestamp":1388585325,"format":"html","reblog-key":"jPfWHFnT","slug":"knock-knock","question":"Knock knock?","answer":"<p>Who is there?<\/p>"}]}'

This comment has been minimized.

@vidbina

vidbina Jan 3, 2014

Contributor

I copied the answer format from my actual Tumblr feed. Took the courtesy to mask the urls for fictitious urls, so I'm pretty sure that this will work. I've confirmed that the code works by importing my real Tumblr feed.

@vidbina

vidbina Jan 3, 2014

Contributor

I copied the answer format from my actual Tumblr feed. Took the courtesy to mask the urls for fictitious urls, so I'm pretty sure that this will work. I've confirmed that the code works by importing my real Tumblr feed.

@@ -125,6 +128,9 @@ def self.post_to_hash(post, format)
unless post["video-caption"].nil?
content << "<br/>" + post["video-caption"]
end
when "answer"

This comment has been minimized.

@vidbina

vidbina Jan 3, 2014

Contributor

Now properly dealing with answer posts

@vidbina

vidbina Jan 3, 2014

Contributor

Now properly dealing with answer posts

Show outdated Hide outdated jekyll-import.gemspec
@@ -6,7 +6,7 @@ Gem::Specification.new do |s|
s.name = 'jekyll-import'
s.version = '0.1.0'
s.date = '2013-12-18'
s.date = '2014-01-01'

This comment has been minimized.

@parkr

parkr Jan 3, 2014

Member

Could you please revert this change?

@parkr

parkr Jan 3, 2014

Member

Could you please revert this change?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 3, 2014

Member

Looks great to me! Just the one reversion above and we'll be 👍 to 🚢.

Member

parkr commented Jan 3, 2014

Looks great to me! Just the one reversion above and we'll be 👍 to 🚢.

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Jan 8, 2014

Member

There's nothing to stop this particular pull request from going in. For next time though, it would be good if unrelated things would be in separate commits and/or pull requests. For example, the shoulda bump is a candidate for this sort of thing. Thanks! ❤️

Member

mattr- commented Jan 8, 2014

There's nothing to stop this particular pull request from going in. For next time though, it would be good if unrelated things would be in separate commits and/or pull requests. For example, the shoulda bump is a candidate for this sort of thing. Thanks! ❤️

@vidbina

This comment has been minimized.

Show comment
Hide comment
@vidbina

vidbina Jan 8, 2014

Contributor

Thanks for the heads up @mattr-. I will pay attention to try issuing changes in separate pull requests. How would you go about proposing a change like this one? The shoulda bump was necessary for me to be able to pass my tests. Separate pull-requests would introduce the possibility of the codebase being modified without the gem deps being updated as well to support the changes (if the code change pull request were to be accepted before the gemspec change pull request). Furthermore I have no idea if Travis CI would have thrown up over my code if shoulda hadn't been updated first. Basing my changes on a pending pull-request could be a solution, but if these changes are in the same branch Github will consolidate it into one pull-request. Should I have proposed the shoulda change in a separate branch? Or do you mean something else?

Contributor

vidbina commented Jan 8, 2014

Thanks for the heads up @mattr-. I will pay attention to try issuing changes in separate pull requests. How would you go about proposing a change like this one? The shoulda bump was necessary for me to be able to pass my tests. Separate pull-requests would introduce the possibility of the codebase being modified without the gem deps being updated as well to support the changes (if the code change pull request were to be accepted before the gemspec change pull request). Furthermore I have no idea if Travis CI would have thrown up over my code if shoulda hadn't been updated first. Basing my changes on a pending pull-request could be a solution, but if these changes are in the same branch Github will consolidate it into one pull-request. Should I have proposed the shoulda change in a separate branch? Or do you mean something else?

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Jan 8, 2014

Member

I totally understand the problem with the dependencies here. If I were doing it, I would have split the shoulda bump into a different branch, made sure all the tests pass, and then make that a pull request.

I would have also split the support for the answer format into a separate branch and pull request too.

I want to stress that this is just one way of working and that there's nothing wrong with working a different way. As project maintainers, we ask for smaller, more focused contributions simply because they're easier for us to see what's happening to the code and digest vs. larger pull requests that do many things at once.

Thanks! 💙

Member

mattr- commented Jan 8, 2014

I totally understand the problem with the dependencies here. If I were doing it, I would have split the shoulda bump into a different branch, made sure all the tests pass, and then make that a pull request.

I would have also split the support for the answer format into a separate branch and pull request too.

I want to stress that this is just one way of working and that there's nothing wrong with working a different way. As project maintainers, we ask for smaller, more focused contributions simply because they're easier for us to see what's happening to the code and digest vs. larger pull requests that do many things at once.

Thanks! 💙

@parkr parkr merged commit 412f005 into jekyll:master Jan 9, 2014

1 check passed

default The Travis CI build passed
Details

parkr added a commit that referenced this pull request Jan 9, 2014

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