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

add leading slash to page.url and post.url #992

Merged
merged 4 commits into from May 5, 2013

Conversation

Projects
None yet
5 participants
@maul-esel
Contributor

maul-esel commented Apr 19, 2013

Liquid:

{% for page in site.pages %}
    {{ page.url }}
{% endfor %}

Example output:

/docs/api/development/items.html
index.html
sitemap.txt
/blog/index.html

This sometimes-present leading slash makes it hard to reliably combine that URL, e.g. with site.url. I had to use the following dirty hack:

{% for page in site.pages %}
    {{ site.url }}{% capture first_letter %}{{ page.url | truncate: 1, '' }}{% endcapture %}{% unless first_letter == '/' %}/{% endunless %}{{ page.url }}
{% endfor %}

I would expect it to be consistent, no matter if that means always or never a leading slash. In this PR however, I removed it (can of course change it if you want me to).

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Apr 19, 2013

I will of course fix the tests once I have feedback on whether to remove or to add a leading slash.

@jhauraw

This comment has been minimized.

Contributor

jhauraw commented Apr 19, 2013

Thank you @maul-esel for your work on Jekyll 👍

I feel the default behavior should be that a leading slash is always expected.

In _config.yml things like baseurl and permalink are typically set with a leading slash. So when combining URL parts the page and post url output should follow the same pattern.

@mattr-

This comment has been minimized.

Member

mattr- commented Apr 19, 2013

IMHO, we need to be consistent and a leading slash always needs to be there.

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Apr 20, 2013

OK, always adding a slash, tests fixed. Should I add tests for this?

@mattr-

This comment has been minimized.

Member

mattr- commented Apr 21, 2013

I don't think you need to add tests for this but others may have different opinions.

@@ -71,8 +71,8 @@ def do_render(post)
@post.read_yaml(@source, file)
assert_equal "my_category/permalinked-post", @post.permalink
assert_equal "my_category", @post.dir
assert_equal "my_category/permalinked-post", @post.url
assert_equal "/my_category", @post.dir

This comment has been minimized.

@parkr

parkr Apr 22, 2013

Member

dir should remain unaffected

This comment has been minimized.

@maul-esel

maul-esel Apr 22, 2013

Contributor

This makes things a little more complicated. I suppose I could move the modification to to_liquid. Otherwise I'd have to clutter up dir really bad.

This comment has been minimized.

@parkr

parkr Apr 22, 2013

Member

no it should be in url like you said. dir shouldn't have to change for this

This comment has been minimized.

@maul-esel

maul-esel Apr 22, 2013

Contributor

Look at dir:

def dir
  File.dirname(url)
end

Apparently, if url has a leading slash, so does File.dirname(url). It could be removed in this method though, but I'm not sure if that would be always consistent with previous behaviour either.

This comment has been minimized.

@parkr

parkr Apr 22, 2013

Member

dir should be relative to your site source. adding a leading slash would be inconsistent with that. dir is used for mkdir_p, so it would be a problem to have the first slash. strip it from dir!

This comment has been minimized.

@maul-esel

maul-esel Apr 22, 2013

Contributor

I can do that, 2 concerns however:

  • several tests expect it to start with a slash
  • isn't it inconsistent with using / as default (source dir) then (as in the page class)?
  • also post.id is modified in some cases

So in some way something gotta change, because currently it's inconsistent and this PR should make it consistent. Question is just: what should be changed?

@parkr

This comment has been minimized.

Member

parkr commented May 4, 2013

I just tried this with @maciakl's sample site and it didn't work because the URL is joined:

{{site.baseurl}}{{p.url}}

where site.baseurl is /, thus resulting in //the-post/. Chrome sees this as protocol-flexible links, so it'll try to send you to http(s)://the-post/ instead of http(s)://your-url/the-post/

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented May 4, 2013

I had used it with site.url which I had set without trailing slash. But if you prefer, I can also change it back to always _remove_ the leading slash (as done originally). I just need a clear guideline: add or remove the slash?

@parkr

This comment has been minimized.

Member

parkr commented May 5, 2013

@mattr-, do you think we should include it or ask to use site.baseurl as the base instead of /?

@parkr

This comment has been minimized.

Member

parkr commented May 5, 2013

Ok jk, add the slash. So this is good to go?

parkr added a commit that referenced this pull request May 5, 2013

Merge pull request #992 from maul-esel/consistent-page.url
add leading slash to page.url and post.url

@parkr parkr merged commit ce999f1 into jekyll:master May 5, 2013

1 check passed

default The Travis build passed
Details

parkr added a commit that referenced this pull request May 5, 2013

@maul-esel maul-esel deleted the maul-esel:consistent-page.url branch May 6, 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.