Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions lib/liquid/standardfilters.rb
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,23 @@ def where(input, property, target_value = nil)
end
end

# Sort elements of an array in numeric order
# provide optional property with which to sort an array of hashes or drops
def sort_numeric(input, property = nil)
ary = InputIterator.new(input)
if property.nil?
ary.sort do |a, b|
Utils.to_number(a) <=> Utils.to_number(b)
end
elsif ary.empty? # The next two cases assume a non-empty array.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this unnecessary (because ary.first.respond_to?(:[]) will be false if ary is empty

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but wouldn't the else branch return nil instead of [] as this branch does?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep I'm an idiot

[]
elsif ary.first.respond_to?(:[]) && !ary.first[property].nil?
ary.sort do |a, b|
Utils.to_number(a[property]) <=> Utils.to_number(b[property])
end
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If none of these if/elsif branches are taken, then it seems like we should treat this as an error. Are you just returning nil in that case to keep consistency with the sort filter? Is consistency with the sort filter more important that providing a clearer error?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we change sort as well?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could result in broken liquid being more obviously broken (i.e. showing Liquid Error: ...). Perhaps we should add logging for in the sort filter so that we will know whether it would break things, then we could turn it into a liquid error if it won't.

end

# Remove duplicate elements from an array
# provide optional property with which to determine uniqueness
def uniq(input, property = nil)
Expand Down
10 changes: 10 additions & 0 deletions test/integration/standard_filter_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,12 @@ def test_sort
assert_equal [{ "a" => 1 }, { "a" => 2 }, { "a" => 3 }, { "a" => 4 }], @filters.sort([{ "a" => 4 }, { "a" => 3 }, { "a" => 1 }, { "a" => 2 }], "a")
end

def test_sort_numeric
assert_equal ['1', '2', '3', '10'], @filters.sort_numeric(['10', '3', '2', '1'])
assert_equal [{ "a" => '1' }, { "a" => '2' }, { "a" => '3' }, { "a" => '10' }],
@filters.sort_numeric([{ "a" => '10' }, { "a" => '3' }, { "a" => '1' }, { "a" => '2' }], "a")
end

def test_sort_with_nils
assert_equal [1, 2, 3, 4, nil], @filters.sort([nil, 4, 3, 2, 1])
assert_equal [{ "a" => 1 }, { "a" => 2 }, { "a" => 3 }, { "a" => 4 }, {}], @filters.sort([{ "a" => 4 }, { "a" => 3 }, {}, { "a" => 1 }, { "a" => 2 }], "a")
Expand Down Expand Up @@ -292,6 +298,10 @@ def test_sort_natural_empty_array
assert_equal [], @filters.sort_natural([], "a")
end

def test_sort_numeric_empty_array
assert_equal [], @filters.sort_numeric([], "a")
end

def test_sort_natural_invalid_property
foo = [
[1],
Expand Down