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

Use post's directory path when matching for the post_url tag #998

Merged
merged 3 commits into from May 5, 2013

Conversation

Projects
None yet
5 participants
@dhcole
Contributor

dhcole commented Apr 25, 2013

Adds support for matching file names of posts in subdirectories. Works with the following:

{% post_url 2013-04-25-post-name %}
{% post_url /2013-04-25-post-name %}
{% post_url blog/2013-04-25-post-name %}
{% post_url /blog/2013-04-25-post-name %}
@parkr

This comment has been minimized.

Member

parkr commented Apr 25, 2013

If you specify category: blog and set the proper permalink, you can achieve this. Or, you could have two separate sources which you compile and push to /blog and /blah, etc.

What is the use-case in which this modification is the only way to achieve your end goal, and what are the results of your examples above? Thanks!

@dhcole

This comment has been minimized.

Contributor

dhcole commented Apr 25, 2013

Hey @parkr:

This allows for posts that are in subdirectories to work with the post_url tag. Since Jekyll supports organizing posts in subdirectories under a _posts/ directory, post_url is incomplete without it.

While these subdirectories don't have any special meaning but for large sites, they are useful to for the owners of sites to help organize content when they're writing a few hundred posts a year and don't want them all in one big list. Permalinks take care of the front end organization, but this is for backend organization. For instance, this is better than this (multiplied every year).

Since Jekyll allows you to puts posts with the same file name in different subdirectories, without this pull request, there's no guarantee that the post_url token will match the right post.

Consider another use case: how we're supporting multilingual sites. We have posts of other languages in language coded subdirectories, like /_posts/2013-01-01-my-post.md and /_posts/es/2013-01-01-my-post.md. This makes it really easy to keep track of the relationships between posts, and by adding categoryies: es to the Spanish language posts, we can make sure the URLs are formatted appropriately too. But we need this change for post_url to work correctly.

So basically, the post_url tag currently does not support all the ways Jekyll does for naming and organizing posts. This pull request fixes that. If a site is not using subdirectories, it behaves as usual.

@parkr

This comment has been minimized.

Member

parkr commented Apr 26, 2013

Ok I'm willing to consider it further. Would you please add tests for your case?

@mattr- and @mojombo's thoughts and I'm in.

@mattr-

This comment has been minimized.

Member

mattr- commented Apr 29, 2013

This needs tests for sure, but I like the idea.

@dhcole

This comment has been minimized.

Contributor

dhcole commented Apr 29, 2013

@parkr @mattr- Thanks. Will work on tests and push them here.

@dhcole

This comment has been minimized.

Contributor

dhcole commented Apr 29, 2013

@parkr The test for this is conflicting with some other tests. Do you know why this line is here? Can it be removed / modified to work with this case?

@parkr

This comment has been minimized.

Member

parkr commented Apr 30, 2013

@dhcole It's there because Jekyll ignores directories which have a valid post name. So if you say File.directory?(File.basename(post)) then you should be fine, probably.

@dhcole

This comment has been minimized.

Contributor

dhcole commented Apr 30, 2013

@parkr alright, I adjusted the tests to account for the extra subfolder. Tests are passing again now.

@date = Time.parse(date)
end
def ==(other)
slug == other.slug &&
path = other.name.split("/")[0...-1].join("/")
otherslug = path != "" ? path + '/' + other.slug : other.slug

This comment has been minimized.

@parkr

parkr Apr 30, 2013

Member

can we turn these two lines into a private method and use that? trying to keep methods as simple as possible :)

@@ -173,7 +173,7 @@ def generate(site)
posts.delete_if { |post| File.directory?(post) }
categories = %w(bar baz category foo z_category publish_test win).sort
assert_equal posts.size, @site.posts.size
assert_equal posts.size + 1, @site.posts.size

This comment has been minimized.

@parkr

parkr Apr 30, 2013

Member

why aren't these two values the same, posts.size and @site.posts.size?

@parkr

This comment has been minimized.

Member

parkr commented Apr 30, 2013

A couple more comments, and I'd like to have @mojombo's input.

@parkr

This comment has been minimized.

Member

parkr commented May 4, 2013

@dhcole I'd love to include this in the 1.0 release (tomorrow night) if you think it's ready. A couple more comments above and I'd love @mattr-'s approval as well.

@dhcole

This comment has been minimized.

Contributor

dhcole commented May 4, 2013

@parkr Cool, I'll work on those two issues today: a private method and clarifying the changes to test, so you can include this tomorrow if you'd like.

@mattr-

This comment has been minimized.

Member

mattr- commented May 5, 2013

I think it's best to push this back to 1.1 for now. I don't want to rush anybody. 1.0.0 has been a long time coming. 1.1 will be made shortly after 1.0.0 I think (based on some other comments I saw) and we'll get it in for that release.

@parkr

This comment has been minimized.

Member

parkr commented May 5, 2013

Nah, I think this is pretty much finished. I know how to fix up the tests and will do so and I'll clean up the ==() method no prob. Unless it brakes something (which is doesn't look like it will), then we're in business.

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

Merge pull request #998 from dhcole/master
Use post's directory path when matching for the post_url tag

@parkr parkr merged commit 290ba13 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

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

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

@fw42

This comment has been minimized.

Contributor

fw42 commented Dec 4, 2013

Hey guys, someone reported a bug in Liquid here, but @parkr said it might be a Jekyll thing related to this PR. Could someone please take a look and let us know if this needs to be fixed in Jekyll or in Liquid?

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