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

Tumblr improvements #27

Closed
wants to merge 4 commits into from
Closed

Conversation

tekkub
Copy link
Contributor

@tekkub tekkub commented May 23, 2013

While importing my tumblr, I ran into a number of reblogged posts that had bad photos in them. This PR includes fixes to retry smaller photos if one fails. I also made the importer a bit more verbose so I could keep track of it working. For folks with lots of photos in their blogs, I made sure that those photos already saved locally are not re-downloaded, making the importer run a lot faster on re-runs.

@parkr
Copy link
Member

parkr commented May 23, 2013

This is great! It looks like some of your changes broke the tests we have in place. Would you mind updating those to comply with your changes? : )

end

end until blog["posts"].size < per_page
Copy link
Member

Choose a reason for hiding this comment

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

This is nifty – I have never seen this post-until syntax before. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was actually in the original code, I just changed the indent here.

Post-until/post-while blocks are pretty awesome for those cases (like this one), where you know you always want to run at least once, and then conditionally re-run in a loop after that initial cycle.

@tekkub
Copy link
Contributor Author

tekkub commented May 24, 2013

I wasn't sure about the tests, I honestly didn't even look at them yet. I got the base stuff working for myself, and wanted to get the PR open so I could start making this whole importer better.

I noticed there seems to be some command line tools for running these scripts, but everyone looks like they just tell people to call ruby directly and then require the importer they want and execute a snippet of code directly. Which is preferred? I think I'd rather use a command line invocation if it's clean and tested.

@mattr-
Copy link
Member

mattr- commented May 24, 2013

I think we'd like to transition folks to the command line tools. The documentation just tells people to call ruby directly and we need to get it updated.

@tekkub
Copy link
Contributor Author

tekkub commented May 24, 2013

Is there a specific invocation for the command line tools? I skimmed the files a bit but didn't see one. Seems like something that should be the very first thing in the README.

@parkr
Copy link
Member

parkr commented May 24, 2013

@tekkub It plugins into jekyll as jekyll import MIGRATOR [opts]. I'm not sure if the command is functional at the moment.

But from here, you can probably run

>> Jekyll::Commands::Import.process(migrator_name, options)

@parkr
Copy link
Member

parkr commented May 28, 2013

@tekkub if you'd prefer i merge and make the modifications I mentioned, 👍. can take a load off your already-full back

@tekkub
Copy link
Contributor Author

tekkub commented May 30, 2013

If you want to do that, it's cool with me. I've left this sitting in my inbox for when I can find time to attack it again, but I'm in SF for work this week.

@parkr
Copy link
Member

parkr commented Jun 15, 2013

I'm going to take care of this now. :)

@parkr parkr closed this in 28c0ce1 Jun 30, 2013
@parkr
Copy link
Member

parkr commented Jun 30, 2013

I've had this tab open for a month – and finally got to fixing the test suite so I could merge. Yay! Thanks again for the fixes, @tekkub. 🍖 🤘

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017
antonizoon pushed a commit to antonizoon/jekyll-import that referenced this pull request Jan 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants