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

Sort collections #2345

Merged
merged 12 commits into from May 10, 2014

Conversation

Projects
None yet
6 participants
@doktorbro
Member

doktorbro commented May 7, 2014

Another try to fix #2328. The tests are coming.

Twin: #2340

@parkr

This comment has been minimized.

Member

parkr commented May 7, 2014

Why can't this be folded into item_property? Why can't item_property handle objects and hashes?

@doktorbro

This comment has been minimized.

Member

doktorbro commented May 7, 2014

@parkr I don’t know much too answer it. I just copied the behaviour from Liquid. Let me write a test to reproduce @budparr’s issue. Then I try both solutions.

@doktorbro

This comment has been minimized.

Member

doktorbro commented May 7, 2014

The Liquid based solution works.

Now trying the item_property

@doktorbro

This comment has been minimized.

Member

doktorbro commented May 7, 2014

item_property works too.

@doktorbro

This comment has been minimized.

Member

doktorbro commented May 8, 2014

I’ve broken it again and get @budparr’s error:

Liquid Exception: undefined method `[]' for #<Jekyll::Document _methods/configuration.md collection=methods> in index.html

So I would stick with our existing item_property. Adding another test …

@doktorbro

This comment has been minimized.

Member

doktorbro commented May 8, 2014

@parkr It’s done.

@parkr

View changes

lib/jekyll/filters.rb Outdated
+ order
else
a[key] <=> b[key]
a_p <=> b_p

This comment has been minimized.

@parkr

parkr May 9, 2014

Member

a_p and b_p are pretty opaque variable names. Would you please extend them to something else? I know they were a and b before – wasn't good before so we may as well take this opportunity to fix it! Thanks!

@parkr

This comment has been minimized.

Member

parkr commented May 9, 2014

No big green merge button. Would you mind rebasing, please? Thank you!

@@ -212,13 +212,16 @@ def sort(input, key = nil, nils = "first")
exit(1)
end
input.sort { |a, b|
if !a[key].nil? && b[key].nil?
input.sort { |apple, orange|

This comment has been minimized.

@doktorbro

doktorbro May 9, 2014

Member

@parkr Do you like fruits? 🍎 🍈

This comment has been minimized.

@mscharley

mscharley May 9, 2014

Contributor

@penibelst How on earth are you comparing apples and oranges? My god man, you'll be the end of us all!

This comment has been minimized.

@doktorbro

doktorbro May 9, 2014

Member

@mscharley 😄 Would you like pork and beef more?

This comment has been minimized.

@parkr

parkr May 9, 2014

Member

It's perfect! Haha :D

@doktorbro

This comment has been minimized.

Member

doktorbro commented May 9, 2014

No big green merge button.

Can you merge it now?

@parkr

This comment has been minimized.

Member

parkr commented May 9, 2014

Yes, mergeable, thank you! @budparr, does this fix your issue?

@budparr

This comment has been minimized.

Contributor

budparr commented May 9, 2014

I'm going to be in a client meeting most of the day, but should be able to test this evening. Thanks, guys!

@budparr

This comment has been minimized.

Contributor

budparr commented May 9, 2014

Oh, it looks like this hasn't been merged, so I'd need to test off a fork of a particular branch?

@mattr-

This comment has been minimized.

Member

mattr- commented May 9, 2014

@budparr yes, you'll need to test the branch of the fork.

@budparr

This comment has been minimized.

Contributor

budparr commented May 10, 2014

works for me. Thanks, gentlemen!

@parkr

This comment has been minimized.

Member

parkr commented May 10, 2014

Wenn das funktioniert, kann ich das mergen :D DANKE @penibelst!!

parkr added a commit that referenced this pull request May 10, 2014

@parkr parkr merged commit 01c09fd into jekyll:master May 10, 2014

1 check passed

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

parkr added a commit that referenced this pull request May 10, 2014

@doktorbro doktorbro deleted the doktorbro:sort-collection branch May 10, 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.