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

Address issue 773 (bugs preventing tumblr.rb from running) #775

Merged
merged 3 commits into from Jan 27, 2013

Conversation

Projects
None yet
5 participants
@metamatt
Copy link
Contributor

commented Jan 22, 2013

One patch to find the _posts directory correctly.

Another patch to truncate long post names correctly.

metamatt added some commits Jan 22, 2013

Look for _posts directory relative to cwd, not relative to tumblr.rb.
That's where we put it earlier, so that's where we'll find it now.

This addresses part of issue #773 (subissue 3).
Fix truncation of overly long post names.
Delete the old truncate_post_name; it was called too late (if url rewriting
is enabled which it is by default), didn't run (it tried to use + to concat
a Fixnum onto a String), and even with those problems fixed, didn't actually
shorten the string enough to use as a pathname.

Instead, apply simple string truncation at the point we generate the slug,
which is used in the filename and is the part that could be unboundedly
long. I arbitrarily chose 200 as the maximum length; even shorter might be
better (really long slugs are just visually ugly); it might also be nicer
to truncate at a hyphen boundary.

This fixes the rest of issue #773 (subissue 4).
urls = Hash[posts.map { |post|
# Create an initial empty file for the post so that
# we can instantiate a post object.
File.open("_posts/tumblr/#{post[:name]}", "w")
tumblr_url = URI.parse(post[:slug]).path
jekyll_url = Jekyll::Post.new(site, dir, "", "tumblr/" + post[:name]).url
jekyll_url = Jekyll::Post.new(site, ".", "", "tumblr/" + post[:name]).url

This comment has been minimized.

Copy link
@parkr

parkr Jan 25, 2013

Member

Maybe Dir.pwd?

@metamatt metamatt referenced this pull request Jan 25, 2013

Closed

Tumblr module path issues #773

@metamatt

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2013

I think that would work fine too. Rhetorical question is what's the difference. The answer is it'll be an absolute pathname instead of relative. The other pieces of code this needs to match up with -- 3 other references to the "_posts" directory in in tumblr.rb, one of which is in the same function self.rewrite_urls_and_redirects() and is visible two lines above in the diff above, and the other two of which are in self.process() -- all use relative paths to find it.

I have a hard time imagining it making a real difference, but I'm happy to change it. Is Jekyll::Post.new supposed to take only absolute paths? Glancing at the other migrators, tumblr.rb is the only one that calls it, and the others just use File.open("_posts/foo") and write directly to it.

Hmm, the argument docs for Post.new are confusing/out of date. They were touched by 4c70c03 and 73d42b2 and which point we're into ancient history, no point worrying about it. But Post constructor is

def initialize(site, source, dir, name)

and all it does with source is

  @base = File.join(source, dir, '_posts')

(and dir is used here, plus split on / to extract categories).

So it looks like any of "", ".", Dir.pwd would work fine.

@parkr

This comment has been minimized.

Copy link
Member

commented Jan 25, 2013

I'm just trying to stay consistent - the old version used the absolute pathname, so I think it safer to use the abs pathname here as well. We just fix where it's pointing to :-)

@metamatt

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2013

OK. I'll test that change and assuming it works, commit to this branch.

Use Dir.pwd instead of "." as the source argument to Post.new, since
this has historically supplied an absolute path in this call.
@metamatt

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2013

Yup, confirmed to work fine.

parkr added a commit that referenced this pull request Jan 27, 2013

Merge pull request #775 from metamatt/issue773
Address issue 773 (bugs preventing tumblr.rb from running)

@parkr parkr merged commit b736676 into jekyll:master Jan 27, 2013

1 check passed

default The Travis build passed
Details

parkr added a commit that referenced this pull request Jan 27, 2013

@sinak

This comment has been minimized.

Copy link

commented on 863643c Jan 29, 2013

This fixed the problem for me.

This comment has been minimized.

Copy link
Owner Author

replied Jan 29, 2013

Glad to hear it. They've committed this upstream now too.

@rathboma

This comment has been minimized.

Copy link
Contributor

commented on c95c5e3 Jan 31, 2013

I think this broke the migrator:

/Users/matthew/.rvm/gems/ruby-1.8.7-p352/gems/jekyll-0.12.0/lib/jekyll/post.rb:44:in `initialize': No such file or directory - /Users/matthew/.rvm/gems/ruby-1.8.7-p352/gems/jekyll-0.12.0/lib/jekyll/migrators/../_posts/tumblr/2013-01-26-auto-indenting-text-in-sublime-text-2.html in /Users/matthew/.rvm/gems/ruby-1.8.7-p352/gems/jekyll-0.12.0/lib/jekyll/migrators/../_posts/tumblr/2013-01-26-auto-indenting-text-in-sublime-text-2.html (Jekyll::FatalException)

Version 0.11 works fine.

This comment has been minimized.

Copy link
Contributor Author

replied Jan 31, 2013

Why do you say that; are you actually running the code with this patch? Offhand that looks like you're hitting exactly the problem I fixed.

This comment has been minimized.

Copy link
Contributor Author

replied Jan 31, 2013

Also, from the fact that the filename in your error message ends in .true, I think you're following an outdated and buggy version of https://github.com/mojombo/jekyll/wiki/blog-migrations (I updated that too; I don't know when/how that final 'true' argument to Tumblr.process was relevant or when it became irrelevant, but you don't want it now).

This comment has been minimized.

Copy link
Contributor

replied Jan 31, 2013

I tried with and without the 'true'.
Hmm. Looks like I'm running 0.12.0 which actually has a third version of this which is broken:

dir = File.join(File.dirname(__FILE__), "..")
...
jekyll_url = Jekyll::Post.new(site, dir, "", "tumblr/" + post[:name]).url

So please discard my original comment. Will build the gem from scratch

This comment has been minimized.

Copy link
Contributor Author

replied Jan 31, 2013

Right. So 0.12.0 looks under __FILE__/.., which fails in a way that matches the error message you posted. I fixed that (for me anyway), with a series of commits including this one. See https://github.com/metamatt/jekyll/commits/issue773 for the whole series. It should work again now (again, it does for me anyway).

The change you're commenting on here isn't in 0.12.0; it's in the current in-progress version. Either check out the source and use that, or use a previous released version like 0.11.0, I suppose.

This comment has been minimized.

Copy link
Contributor

replied Jan 31, 2013

Yep, sorry for confusing things.

@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.
You can’t perform that action at this time.