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

Fixing pagination on windows #1063

Merged
merged 2 commits into from May 22, 2013

Conversation

Projects
None yet
6 participants
@kelsin
Contributor

kelsin commented May 8, 2013

This is a duplicate of #1058 which was in a funky state... it didn't seem to recall what branch I made the push from, so I'm recreating it

Problem
On windows the File.basename function will separate paths on : so File.basename('path:num') == 'path'.

The result currently is that on >= 1.0.0 (on windows) pagination just doesn't work. We end up with the last page being rendered to _site/page/index.html and no other page folders at all. This commit fixes that and my pagination on my site behaves like it did in 0.12.1.

Code Change
This fix substitutes :num for the page number first and calls File.basename after so that : is no longer in the path.

Tests
The test provided doesn't break on master for linux and mac but does on windows.

Right now my rake test is clean on my mac for this branch but dirty for other reasons on windows. I confirmed that it's not more broken than master however.

On my mac and windows machines rake feature has many failing tests but on both machines no more failures than on master. I haven't confirmed exactly why to find out if my mac is setup wrong. I'm assuming the breaks on windows are normal non-unix type of things.

@kelsin

This comment has been minimized.

Show comment
Hide comment
@kelsin

kelsin May 8, 2013

Contributor

I added a second commit to the original pull request that further clarifies how paginate_path currently works.

Right now in the code you use File.basename and the directory structure of the index page to make the full path:
https://github.com/mojombo/jekyll/blob/master/lib/jekyll/generators/pagination.rb#L40

This is also tested by your features:
https://github.com/mojombo/jekyll/blob/master/features/pagination.feature#L33

So it looks like File.basename is the right thing to do unless you want to refactor other parts of the code. The purpose of this pull request however still stands since on windows we need to substitute :num before calling File.basename.

Contributor

kelsin commented May 8, 2013

I added a second commit to the original pull request that further clarifies how paginate_path currently works.

Right now in the code you use File.basename and the directory structure of the index page to make the full path:
https://github.com/mojombo/jekyll/blob/master/lib/jekyll/generators/pagination.rb#L40

This is also tested by your features:
https://github.com/mojombo/jekyll/blob/master/features/pagination.feature#L33

So it looks like File.basename is the right thing to do unless you want to refactor other parts of the code. The purpose of this pull request however still stands since on windows we need to substitute :num before calling File.basename.

@ddatsh

This comment has been minimized.

Show comment
Hide comment
@ddatsh

ddatsh May 22, 2013

thanks!my problem solved

ddatsh commented May 22, 2013

thanks!my problem solved

format.sub(':num', num_page.to_s)
format = site_config['paginate_path']
format = format.sub(':num', num_page.to_s)
File.basename(format)

This comment has been minimized.

@parkr

parkr May 22, 2013

Member

weird, we shouldn't even be doing this basename call. if you set paginate_path = "/blog/page/:num and call this with num_page = 3 it should return /blog/page/3 right?

@parkr

parkr May 22, 2013

Member

weird, we shouldn't even be doing this basename call. if you set paginate_path = "/blog/page/:num and call this with num_page = 3 it should return /blog/page/3 right?

This comment has been minimized.

@parkr

parkr May 22, 2013

Member

that's the actual path

@parkr

parkr May 22, 2013

Member

that's the actual path

This comment has been minimized.

@kelsin

kelsin May 22, 2013

Contributor

I already answered this question in my pull request. The code that uses this result is:
https://github.com/mojombo/jekyll/blob/master/lib/jekyll/generators/pagination.rb#L40

If you wanted this function to return the whole path then you need to rework paginate to not join this with the page.dir

This is also already currently tested with your tests:
https://github.com/mojombo/jekyll/blob/master/features/pagination.feature#L33

@kelsin

kelsin May 22, 2013

Contributor

I already answered this question in my pull request. The code that uses this result is:
https://github.com/mojombo/jekyll/blob/master/lib/jekyll/generators/pagination.rb#L40

If you wanted this function to return the whole path then you need to rework paginate to not join this with the page.dir

This is also already currently tested with your tests:
https://github.com/mojombo/jekyll/blob/master/features/pagination.feature#L33

This comment has been minimized.

@kelsin

kelsin May 22, 2013

Contributor

I should add that all I want to do is make your current gem work on windows. If you want to rework this section of code, that's fine go ahead. If you're going to leave it as is, we need this fix in order to make it work properly on windows.

Whether or not you feel basename is proper here is beyond the scope of this request. It currently works that way so I didn't change it.

@kelsin

kelsin May 22, 2013

Contributor

I should add that all I want to do is make your current gem work on windows. If you want to rework this section of code, that's fine go ahead. If you're going to leave it as is, we need this fix in order to make it work properly on windows.

Whether or not you feel basename is proper here is beyond the scope of this request. It currently works that way so I didn't change it.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 22, 2013

Member

Cool, then 👍 from me. @mattr-?

Member

parkr commented May 22, 2013

Cool, then 👍 from me. @mattr-?

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- May 22, 2013

Member

👍

Member

mattr- commented May 22, 2013

👍

mattr- added a commit that referenced this pull request May 22, 2013

@mattr- mattr- merged commit 1eab13c into jekyll:master May 22, 2013

1 check passed

default The Travis CI build passed
Details

mattr- added a commit that referenced this pull request May 22, 2013

@kelsin

This comment has been minimized.

Show comment
Hide comment
@kelsin

kelsin May 22, 2013

Contributor

Yay thanks guys :) Loving working with jekyll so far!

Contributor

kelsin commented May 22, 2013

Yay thanks guys :) Loving working with jekyll so far!

@coderabbi

This comment has been minimized.

Show comment
Hide comment
@coderabbi

coderabbi Aug 17, 2014

This appears to be an existing issue in 2.2.
Under Windows, pagination fails to create page2.html, etc. and renders the last page as index.html

This appears to be an existing issue in 2.2.
Under Windows, pagination fails to create page2.html, etc. and renders the last page as index.html

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 17, 2014

Member

@coderabbi Can you please open a new issue on jekyll/jekyll-paginate for this? It's a separate repo containing our pagination code. Thanks!

Member

parkr commented Aug 17, 2014

@coderabbi Can you please open a new issue on jekyll/jekyll-paginate for this? It's a separate repo containing our pagination code. Thanks!

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