require 'pathname' to fix 'NameError' exceptions #1255

Closed
wants to merge 1 commit into
from

Projects

None yet

6 participants

@maul-esel
Contributor

The tests began to fail for me locally recently, with several NameError exceptions for Pathname, while they were running fine on travis. Adding this directive fixed that.

Might as well be an issue with my system only, in which case you can close this.

@parkr
Member
parkr commented Jul 2, 2013

All the tests still pass for me. What ruby version were you dev-ing against?

@maul-esel
Contributor

It was working again the next day and works ever since. No idea to what the cause was. 😕

@maul-esel maul-esel closed this Jul 5, 2013
@maul-esel maul-esel deleted the maul-esel:require-pathname branch Jul 5, 2013
@mrueg
Contributor
mrueg commented Jul 31, 2013

Tests fail for jekyll-1.1.2 currently with the same problem. Is it possible to apply this anyway?

@parkr
Member
parkr commented Jul 31, 2013

@mrueg What Ruby version?

@ddavison

Using latest ruby candidate, ruby 2.0.0p247 (2013-06-27 revision 41674) [x86_64-linux]

I can confirm that this fixes this issue

@parkr
Member
parkr commented Aug 21, 2013

Did Ruby 2.0 change this behaviour?

@penibelst
Member

If I’m using Ruby 1.8.7 (2011-06-30 patchlevel 352) [i686-linux] I still get these 9 errors until I upgrade to Ruby 2.0.0?

@parkr
Member
parkr commented Sep 13, 2013

Weird. Why do you think it'd work for me on 1.9.3-p448 if it fails for you on < 2.0?

@penibelst
Member

Might as well be an issue with my system only, in which case you can close this.

@maul-esel My system produces the same 9 exceptions. After adding your patch the tests run without errors. I use a default Ubuntu 12.04 (LTS).

$ ruby -v
ruby 1.8.7 (2011-06-30 patchlevel 352) [i686-linux]

$ bundle exec rake test
/usr/bin/ruby1.9.1 -I"lib:lib:test" -I"/var/lib/gems/1.9.1/gems/rake-10.0.4/lib" "/var/lib/gems/1.9.1/gems/rake-10.0.4/lib/rake/rake_test_loader.rb" "test/**/test_*.rb" 
Run options: 

# Running tests:

.....................................................................................................................................................................................................................................................................................

Finished tests in 25.053895s, 11.0562 tests/s, 17.7218 assertions/s.

277 tests, 444 assertions, 0 failures, 0 errors, 0 skips
@parkr
Member
parkr commented Sep 14, 2013

On a fresh clone of Jekyll, Ruby v1.8.7-p371, all the tests run just fine! This is crazy weird.

@maul-esel
Contributor

I tested on:

ruby result
rvm: ruby-1.8.7-p374 fine
rvm: ruby-1.9.3-p448 bad
rvm: ruby-2.0.0-p247 fine
system: ruby 1.9.3p194 bad

Running on Ubuntu 13.04.

@penibelst
Member

I think I get it. The issue starts in /lib/jekyll/generators/pagination.rb#L130:

    def self.in_hierarchy(source, page_dir, paginate_path)
      # line 130
      return false if paginate_path == File.dirname(paginate_path)
      # line 131
      return false if paginate_path == Pathname.new(source).parent
      ...
    end

My paginate_path is /jekyll/test/source, but File.dirname(paginate_path) returns /jekyll/test. So we go to the next line and try to create a new object Pathname.new(source) which raise an exception if no pathname is required at the top of the file.

It looks like on some systems paginate_path always equals File.dirname(paginate_path) and Jekyll never goes to the line 131 and never try to create a Pathname object.

Please correct me if I’m wrong.

@maul-esel
Contributor

Sounds sensible. So if that's the case, the require 'pathname' should¹ actually be there (or in lib/jekyll.rb).


¹ I had always thought it should be unnecessary and it's some ruby fail.

@parkr
Member
parkr commented Sep 16, 2013

Ok then we need to add require pathname to lib/jekyll.rb under stdlib.

@mattr-
Member
mattr- commented Sep 17, 2013

Done.

@parkr
Member
parkr commented Sep 17, 2013

Would you mind linking to the PR/revision?

@mattr-
Member
mattr- commented Sep 17, 2013

Never! 😉

Yeah, I need to add something to history for this. Got distracted.

@parkr
Member
parkr commented Sep 17, 2013

❤️ thank you!!!

@mattr- mattr- added a commit that referenced this pull request Sep 17, 2013
@mattr- mattr- Update history to reflect merge of #1255 f7beb6b
@penibelst
Member

Much better now.

@parkr
Member
parkr commented Sep 17, 2013

Related commit: 454e038

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment