Skip to content
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

Allow sorting by custom properties #1849

Merged
merged 8 commits into from Dec 31, 2013

Conversation

Projects
None yet
7 participants
@afeld
Copy link
Contributor

afeld commented Dec 19, 2013

Fixes #1802.

Added a failing test to support sort:'some-custom-property'. Have spent a couple hours poking around Jekyll/Liquid to figure out how to make those properties available to the filter, but haven't been able to figure it out yet.

Presumably a fix would work for other filters as well? Not sure how to test more generically.

P.S. These things are called "variables" in http://jekyllrb.com/docs/frontmatter/, but are called "properties" in the Liquid docs (e.g. in sort). Sorry for my inconsistency... I'll clean up later.

/cc @parkr @Haacked

@afeld

This comment has been minimized.

Copy link
Owner Author

afeld commented on c2b7504 Dec 19, 2013

@afeld

This comment has been minimized.

Copy link
Contributor Author

afeld commented Dec 19, 2013

K, ready for 👀!

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 19, 2013

Coverage Status

Coverage decreased (-0.09%) when pulling c2b7504 on afeld:sort-attributes into 12a55b8 on jekyll:master.

#
# Returns the String value or nil if the property isn't included.
def [](property)
data[property]

This comment has been minimized.

@parkr

parkr Dec 20, 2013

Member

My only worry with this is that then folks won't be able to sort by layout, name, dir, etc. data doesn't encase everything, unfortunately.

This comment has been minimized.

@afeld

afeld Dec 20, 2013

Author Contributor

Not entirely true, since some of those will be covered by the respond_to?. I'll add some more tests and make #[] cover all of the attributes we could want.

@doktorbro doktorbro referenced this pull request Dec 20, 2013

Closed

Kill coveralls #1857

@afeld

This comment has been minimized.

Copy link
Contributor Author

afeld commented Dec 22, 2013

K, made (most) other properties accessible via #[] now too. This gave me a lot of ideas about refactoring how data is passed around, but I'll leave those for separate PRs.

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Dec 22, 2013

@afeld Awesome! Looks like Travis doesn't like your latest changes. Would you mind fixing up those broken tests? Thanks!

@afeld

This comment has been minimized.

Copy link
Contributor Author

afeld commented Dec 23, 2013

Fixed!

#
# Returns the String value or nil if the property isn't included.
def [](property)
if self.class::ATTRIBUTES_FOR_LIQUID.include?(property)

This comment has been minimized.

@parkr

parkr Dec 23, 2013

Member

What do you think about this:

if respond_to?(property.to_sym)
  send(property.to_sym)
elsif respond_to?(:data)
  data[property]
else
  ""
end

Or something? What kind of precedent are you looking to take advantage of?

This comment has been minimized.

@afeld

afeld Dec 23, 2013

Author Contributor

I was looking at the other uses of ATTRIBUTES_FOR_LIQUID, which I assume is a security measure so that arbitrary methods can't be called through Liquid.

This comment has been minimized.

@parkr

parkr Dec 23, 2013

Member

Mm, the security vulnerabilities are definitely an important concern. What about using to_liquid.include? instead?

This comment has been minimized.

@afeld

afeld Dec 23, 2013

Author Contributor

Because then it's needlessly constructing the whole hash, where only the keys and the single value are needed. I think there's a lot of refactoring that could be done around to_liquid, but I'm hoping to keep this PR from getting too complicated.

@@ -9,6 +9,7 @@ class Post
EXCERPT_ATTRIBUTES_FOR_LIQUID = %w[
title
url
dir

This comment has been minimized.

@parkr

parkr Dec 23, 2013

Member

Why do you want to expose this to Liquid?

This comment has been minimized.

@afeld

afeld Dec 23, 2013

Author Contributor

Same as above

path
url

This comment has been minimized.

@parkr

parkr Dec 23, 2013

Member

Why do you want to expose dir and name to Liquid?

This comment has been minimized.

@afeld

afeld Dec 23, 2013

Author Contributor

You had mentioned them before... I was basically trying to expose all the variables available on 'page', going off the documentation on the site.

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Dec 23, 2013

Looks pretty good to me. @mattr-, would love to get your 👀 on this.

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Dec 24, 2013

I'm 👍 on this.

Just want @mattr-'s input then we'll be good to go.

@ghost ghost assigned mattr- Dec 26, 2013

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented Dec 31, 2013

Looks fine to me. Although I beginning to think I'm the only person who still prefers hashrockets to the new ruby 1.9 syntax. 😃

mattr- added a commit that referenced this pull request Dec 31, 2013

@mattr- mattr- merged commit a2fd8ba into jekyll:master Dec 31, 2013

1 check passed

default The Travis CI build passed
Details

mattr- added a commit that referenced this pull request Dec 31, 2013

@qrohlf

This comment has been minimized.

Copy link

qrohlf commented Apr 29, 2014

This is in 2.0.0.rc1 latest, right? I've been struggling with trying to sort my chapters collection by a custom index property all morning, and if @afeld or anyone else could give me a nudge in the right direction I'd be infinitely grateful.

Here's what I've got right now:

(default.html template snippet)

{% assign contents = site.chapters | sort:'index' %}
{% for chapter in contents %}
  <a href="{{site.baseurl}}{{chapter.url}}" class="list-group-item">{{chapter.index}}. {{ chapter.subtitle }}</a>
{% endfor %}
@parkr

This comment has been minimized.

Copy link
Member

parkr commented Apr 29, 2014

Maybe @penibelst is experiencing the same issue with assign and sort? Ref: #2275

@doktorbro

This comment has been minimized.

Copy link
Member

doktorbro commented Apr 29, 2014

@qrohlf Is contents empty after assign?

@qrohlf

This comment has been minimized.

Copy link

qrohlf commented Apr 29, 2014

@penibelst yeah, it's empty.

@doktorbro

This comment has been minimized.

Copy link
Member

doktorbro commented Apr 29, 2014

@qrohlf I would say the sorting feature is broken.

@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.
You can’t perform that action at this time.