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

Relative posts should never fail to build, even if @dir or @name is nil #1976

Merged
merged 3 commits into from Feb 13, 2014

Conversation

Projects
None yet
5 participants
@parkr
Member

parkr commented Jan 22, 2014

Fixes #1963

  • Implement
  • ADD TESTS WHY WEREN'T THEY THERE WUT

@ghost ghost assigned mattr- Jan 22, 2014

@mattr-

This comment has been minimized.

Member

mattr- commented Jan 22, 2014

I think it would be better to figure out how nil is getting into that part of the code and find a way to prevent it. The lack of a to_s call is just masking the real error.

@ghost ghost assigned parkr Jan 22, 2014

@parkr

This comment has been minimized.

Member

parkr commented Jan 22, 2014

The lack of a to_s call is just masking the real error.

@mattr- There is no real error – if @dir is nil, it means "put me in the root of the destination directory!" If @name is nil, we have a bigger problem, yes, but I am fairly certain it's @dir that comes in as nil.

@ghost ghost assigned mattr- Jan 23, 2014

@tamouse

This comment has been minimized.

tamouse commented on lib/jekyll/page.rb in 606c525 Jan 30, 2014

after you force it to a string, can x ever be nil?

@tamouse

This comment has been minimized.

tamouse commented on lib/jekyll/post.rb in 606c525 Jan 30, 2014

same question: can x ever be nil?

File.join([
@dir.to_s,
@name.to_s
].reject {|x| x.nil? || x.empty?})

This comment has been minimized.

@tamouse

tamouse Jan 30, 2014

Also, this seems DRYer:

File.join([@dir,@name].map(&:to_s).reject{|x| x.empty?}

This comment has been minimized.

@parkr

parkr Jan 30, 2014

Member

Agreed! I'll fix it up.

@ivantsepp

This comment has been minimized.

Contributor

ivantsepp commented on lib/jekyll/page.rb in baabe7e Feb 1, 2014

Is the reject call to filter out empty strings necessary? It seems that File.join handles empty strings

This comment has been minimized.

Member

parkr replied Feb 1, 2014

Does it? We want to make sure there is only /. Can you link me to the docs wherein you found this?

This comment has been minimized.

Contributor

ivantsepp replied Feb 1, 2014

Originally, I did some File.join tests on my machine to arrive at the conclusion that it handles empty strings (no duplicate /). I couldn't find any documentation on that behavior however 😞.

But, I did find some specs in RubySpec dealing with empty string cases.

This comment has been minimized.

Member

parkr replied Feb 2, 2014

We don't want the leading / if @dir is nil.

This comment has been minimized.

Contributor

ivantsepp replied Feb 2, 2014

Ahhh I see, then the reject filter is needed. 👍 !

This comment has been minimized.

tamouse replied Feb 2, 2014

I didn't see a way out of this since either of those could potentially be nil or empty. At least doing the mapping to_s ensures that if it was nil, it will now be empty.

mattr- added a commit that referenced this pull request Feb 13, 2014

mattr- added a commit that referenced this pull request Feb 13, 2014

Merge pull request #1976 from jekyll/fix-pagination-issue
Relative posts should never fail to build, even if @dir or @name is nil

@mattr- mattr- merged commit ac7bed3 into master Feb 13, 2014

1 check passed

default The Travis CI build passed
Details

@mattr- mattr- deleted the fix-pagination-issue branch Feb 13, 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.