Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix broken post_url with posts with a time in their YAML front matter. #831

Merged
merged 3 commits into from Mar 4, 2013

Conversation

Projects
None yet
3 participants
Contributor

dhilgarth commented Mar 3, 2013

This pull requests fixes the post_url liquid tag. It failed as soon as the YAML front matter contained a time. Especially when using Octopress, this is a problem, because it defaults to including time.

@dhilgarth dhilgarth referenced this pull request in imathis/octopress Mar 3, 2013

Closed

How to add internal link in post? #544

Fix invalid ordering of posts published on the same day and move post…
…_url specific comparison of posts where it belongs: Into post_url
Contributor

dhilgarth commented Mar 3, 2013

Sorry about using three commits for such a simple change. The last commit effectively reverts the changes done by the first two. In effect, I only changed post_url.rb.

Owner

parkr commented Mar 3, 2013

Hey @dhilgarth, thanks for your contribution!

In Post#<=>, we compare the date, then the slug. Is the problem you're talking about related to a post's full date (including time) not being the same and thus post_url would return nothing?

Contributor

dhilgarth commented Mar 3, 2013

@parkr, thanks a lot for looking at this request that quickly!

The problem is the following:
post_url defines a PostComparer class that exposes two attributes (is that what they are called in Ruby?):
date and slug.
It gets the values for those attributes from the string specified in the post_url liquid tag. This string only contains a date, not a time.
Imagine a post with a filename 2013-03-03-blog-organization.md and a front matter containing this: date: 2013-03-03 13:17.
I want to reference this with a post_url tag like this: {% post_url 2013-03-03-blog-organization %}.
The current version of jekyll reports that it couldn't find a matching post, because it compares 2013-03-03 13:17 with 2013-03-03 00:00. The fact that the slugs are the same doesn't matter, the comparison fails before that.

However, changing Post#<=> wasn't the solution, because it is used for ordering the posts in the index.html and other places. When publishing multiple posts on the same day, my first commit lead to an ordering by slug, not by time of day. That's the reason why the third commit reverts this and doesn't use Post#<=> at all.

Owner

parkr commented Mar 4, 2013

Got it! I'd love to keep comparison encapsulated in the Post model wherever possible, so something like loose_compare would be preferable to a big ol' if statement. What do you think about that?

Contributor

dhilgarth commented Mar 4, 2013

I am not really sure this comparison belongs to Post. Right now, it looks to me as if this comparison is specific to post_url and not to Post´. If you agree, we could encapsulate this comparison somewhere insidepost_url`.

But if you really think it belongs to Post - maybe because some other tag might need to use this comparison, too - I will gladly add it.

Your call ☺

Owner

parkr commented Mar 4, 2013

Nah, you're absolutely right – more important to be specific to the usage domain.

Last request would be to observe the line width limit of 80 columns. :)

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

Merge pull request #831 from dhilgarth/feature/fix_post_url
Fix broken post_url with posts with a time in their YAML front matter.

@parkr parkr merged commit e98053e into jekyll:master Mar 4, 2013

1 check passed

default The Travis build passed
Details
Owner

parkr commented Mar 4, 2013

I'll take care of it :)

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

Contributor

dhilgarth commented Mar 4, 2013

Ah, I just pushed another commit that moved the comparison to a method and took care of the long line.
Even if it won't be used now, I would be interested if that's something a ruby pro would do - I mean the many return ... unless ....

Contributor

dhilgarth commented Mar 4, 2013

BTW: I think this old pull request can now be closed, too.

Owner

parkr commented Mar 4, 2013

Took care of it – I figured that comparison was specific to that one use-case and therefore it makes sense to leave it in post_url.

Great catch! I'll close up #517.

Contributor

dhilgarth commented Mar 4, 2013

Yes, I left that comparison in post_url, too. Just moved it to the PostComparer class. I think that's where this comparison really belongs to.

----- Reply message -----
From: "Parker Moore" notifications@github.com
To: "mojombo/jekyll" jekyll@noreply.github.com
Cc: "Daniel Hilgarth" d.hilgarth@fire-development.com
Subject: [jekyll] Fix broken post_url with posts with a time in their YAML front matter. (#831)
Date: Mon, Mar 4, 2013 03:45

Took care of it – I figured that comparison was specific to that one use-case and therefore it makes sense to leave it in post_url.

Great catch! I'll close up #517mojombo#517.


Reply to this email directly or view it on GitHubhttps://github.com/mojombo/jekyll/pull/831#issuecomment-14361745.

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