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

Added path in url. #536

Merged
merged 3 commits into from Mar 10, 2013

Conversation

Projects
None yet
6 participants
@fotos
Contributor

fotos commented Apr 9, 2012

Page#dir was returning the wrong dir ('/') for pages in directories.

Also #url was returning the wrong path for pages in directories.

Actually I wrote a fix and the tests but then I saw the list of 49 (!) pending pull requests and went through the list to find if anyone else was having the same problem. Issue #506 was a similar solution to mine but a bit cleaner and decided to use MrWerewolf's soluaion but with the tests I have already written.

Some credit should be given to MrWerewolf.

Added path in url.
Page#dir was returning the wrong dir ('/') for pages in directories.
@danielgrieve

This comment has been minimized.

Contributor

danielgrieve commented Feb 26, 2013

I'd like this to be merged into master if it can. How can we make this happen?

@mattr-

This comment has been minimized.

mattr- commented on test/test_page.rb in 4090500 Feb 26, 2013

should "write the index properly" is a much better description for this test IMHO.

should "properly write the index file" would also work.

Take your pick 😄

@fotos

This comment has been minimized.

Contributor

fotos commented Feb 26, 2013

@mattr- thanks for your comment! If you look at the context above the tests I added, it uses the same format: "write properly [...]". So I decided to go with the flow and respect the file conventions even tho they don't spell out properly (they also don't spell out properly above as well).

Well, is changing a test description going to land this into master? If you are so positive about this, then I'll go ahead and update the PR.

@mattr-

This comment has been minimized.

Member

mattr- commented Feb 27, 2013

If it fits with the rest of the file, then there's no need to change it.

@parkr or @mojombo this gets a 👍 from me. What do you think?

should "return dir correctly for index page" do
@page = setup_page('/contacts', 'index.html')
assert_equal '/contacts', @page.dir

This comment has been minimized.

@parkr

parkr Feb 27, 2013

Member

This test assertion doesn't have the trailing / while the previous one doesn't. What's up with that? Why wouldn't an index page also deserve this trailing slash?

This comment has been minimized.

@fotos

fotos Feb 27, 2013

Contributor

@parkr well it's been almost a year (wow... 11 months ago) since I submitted this. 😄

OK, I dug through the code again. Nice catch! But it doesn't seem to matter since this is the directory where the file / page will be put into and not a (pretty) URL.

It appears it's not related to this commit but it just surfaced from the tests. The "issue" lies within #dir.

For example when the URL is /contacts/index.html, it doesn't end with a slash and File#basename returns /contacts. When the URL is /contacts/bar/ it ends with a URL and the dir is returned as is (same at the URL).

Honestly I'm more concerned with the "A Page processing pages with pretty url style in a directory hierarchy should create url based on filename" test which should return "/contacts/" as the pretty URL for the index file in a directory.

I will investigate this a bit more when I get home. Looks like that #dir and/or #template might have to be changed a bit.

This comment has been minimized.

@parkr

parkr Feb 27, 2013

Member

Let me know what you find out.

In terms of the wait, I have only been a moderator for two months and I think I've done a pretty good job of dealing with crucial issues. I look at every notification from this repo. Once it's convincing enough, I'll accept it. You can expect more responsiveness from me.

Fix pretty url style paths.
Ignore the basename if the page is an index page, preserve it if it's just an
html page and use the full path in every other case.
@fotos

This comment has been minimized.

Contributor

fotos commented Feb 28, 2013

Hey @parkr thanks for your feedback!

First things first, I meant absolutely no disrespect, nor did I imply that you guys are slow at responding to pull requests or anything. Sorry if it sounded that way! All I wanted to say was that a year has passed... I didn't even remember this pull request existed, nor how it fixed the issue at hand and I had to sift through this old code again to understand how it worked, which is not always a pleasant thing (as I guess you already know). I was positively surprised that there was interest for this PR after a year! 😲

I updated the tests, per the above discovery and investigation, and made some changes in #template, all pushed as a separated commit. If it looks ok, that I can squash them together and rebase them to a more recent version of upstream (mojombo) master for cleaner history / easier merging. Or you can merge as is. Let me know what you prefer.

Thanks for looking into this. Really appreciated!

if self.site.permalink_style == :pretty
if index? && html?
"/:path/"
elsif html?

This comment has been minimized.

@parkr

parkr Feb 28, 2013

Member

Indentation issue here.

@fotos

This comment has been minimized.

Contributor

fotos commented Mar 9, 2013

@parkr fixed. I did this patch in a different workstation which didn't use soft-tabs and I missed this one.

@parkr

This comment has been minimized.

Member

parkr commented Mar 9, 2013

Awesome. This looks ready to me.

@mojombo

This comment has been minimized.

Contributor

mojombo commented Mar 9, 2013

This looks great. @parkr +1 to merge.

parkr added a commit that referenced this pull request Mar 10, 2013

@parkr parkr merged commit 725b127 into jekyll:master Mar 10, 2013

1 check passed

default The Travis build passed
Details

parkr added a commit that referenced this pull request Mar 10, 2013

@fotos fotos deleted the fotos:fix_page_dir branch Mar 10, 2013

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