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

Override the sort filter #2299

Merged
merged 14 commits into from May 6, 2014

Conversation

Projects
None yet
4 participants
@doktorbro
Copy link
Member

doktorbro commented May 5, 2014

This is a try to get rid of #2287. The behavior of nil values is borrowed from SQL. See related discussion in Shopify/liquid#355. Fixes #2239.

# nils_last - nils appear after non-nil values in the sort ordering
#
# Returns the filtered array of objects
def sort(input, key = nil, nils_last = false)

This comment has been minimized.

@doktorbro

doktorbro May 6, 2014

Author Member

Another name for nils order would be nils with two string options: 'first' (default) or 'last'.

This comment has been minimized.

@parkr

parkr May 6, 2014

Member

I like a string better :)

This comment has been minimized.

@doktorbro

doktorbro May 6, 2014

Author Member

Okay, the string is coming. Like this:

{{ site.pages | sort: 'title', 'first' }}
assert_equal [2, 10], @filter.sort([10, 2])
assert_equal [{"a" => 2}, {"a" => 10}], @filter.sort([{"a" => 10}, {"a" => 2}], "a")
assert_equal ["10", "2"], @filter.sort(["10", "2"])
assert_equal [{"a" => "10"}, {"a" => "2"}], @filter.sort([{"a" => "10"}, {"a" => "2"}], "a")

This comment has been minimized.

@parkr

parkr May 6, 2014

Member

Would you mind adding another line with text, like sorting the names of dogs or something? Dealing with mostly numbers in these tests. Curious how this will sort capitals vs. lower-case letters, for example.

@parkr

This comment has been minimized.

Copy link
Member

parkr commented May 6, 2014

If this isn't right for Liquid, then I'm happy to add it here.

@parkr

This comment has been minimized.

Copy link
Member

parkr commented May 6, 2014

@benbalter does this LGTY? I am happy to add this to the 2.0.0 release post today if I get a strong 👍 from you.

@doktorbro

This comment has been minimized.

Copy link
Member Author

doktorbro commented May 6, 2014

@parkr What do you think about the third argument? Boolean or string?

@parkr

This comment has been minimized.

Copy link
Member

parkr commented May 6, 2014

Would you also please add a tr to the "Filters" table on http://jekyllrb.com/docs/templates/ to document this?

@doktorbro

This comment has been minimized.

Copy link
Member Author

doktorbro commented May 6, 2014

Yes, I’m writing the documentation.

doktorbro added some commits May 6, 2014

<tr>
<td>
<p class="name"><strong>Sort</strong></p>
<p>Sort an array. Optional arguments for hashes: 1.&nbsp;property name 2.&nbsp;nils order (<em>first</em> or <em>last</em>).</p>

This comment has been minimized.

@parkr

parkr May 6, 2014

Member

What are the nbsp's for? Why won't regular spaces work here?

This comment has been minimized.

@doktorbro

doktorbro May 6, 2014

Author Member

nbsp ensures that the number doesn’t end the line.

This comment has been minimized.

@parkr

parkr May 6, 2014

Member

Oh, interesting! Ok cool. 👍

@parkr

This comment has been minimized.

Copy link
Member

parkr commented May 6, 2014

LGTM. @mattr- and @gregose, would you please take a look and give me your 👍? Thanks!

@parkr parkr added this to the 2.0 milestone May 6, 2014

@doktorbro

This comment has been minimized.

Copy link
Member Author

doktorbro commented May 6, 2014

I am happy to add this to the 2.0.0 release post today

Me too.

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented May 6, 2014

I'm ok with this. We'll need to make sure that we do a good job of documenting the sorting behavior with this filter. Reading the cukes (before I read the rest), I thought this wasn't sorting correctly. 😃

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented May 6, 2014

@doktorbro

This comment has been minimized.

Copy link
Member Author

doktorbro commented May 6, 2014

@mattr- This implementation acts exactly like the Liquid sort filter except it doesn’t fail on non-existent properties. People with SQL background are familiar with the nils order option. I hope one day Liquid will expand his sort filter in the same manner, and Jekyll can just use Liquids version again.

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented May 6, 2014

Understood. How do we make sure that our users know that's how the filter is supposed to behave though? That's my concern. They're not going to go digging through code and open issues to figure it out.

The only thing I'm asking for (and it doesn't even hold up merging this PR) is that the behavior is documented so that people know what to expect.

@doktorbro

This comment has been minimized.

Copy link
Member Author

doktorbro commented May 6, 2014

the behavior is documented so that people know what to expect.

Sure. I’ve documented all I can. Should I add a link to Shopify’s description?

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented May 6, 2014

I'm sorry. I totally missed it before. You did document it. Thanks! ❤️ 💙 💛

mattr- added a commit that referenced this pull request May 6, 2014

@mattr- mattr- merged commit 5c109ee into jekyll:master May 6, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

mattr- added a commit that referenced this pull request May 6, 2014

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