Fix for loop with reversed and limit.

[joost]

# array being [1,2,3]
for item in array reversed limit:2

currently returns [2,1]. This fix makes it more logically: [3,2].

[joost]

A (temporary) fix in your liquid templates would be:

{% assign array = array | reverse %}

[fw42]

Hm. Good catch. I agree that it makes more sense like this. But this fix is not strictly backwards compatible...

@Shopify/liquid, think we can ship this without breakage?

[dylanahsmith]

This is an in place operation on the collection itself, which seems like it could have some nasty side effects. Also, the collection may not be an array at this point, since Utils.slice_collection supports anything with the method each (see Liquid::Utils.slice_collection_using_each).

[dylanahsmith]

I agree that it makes more sense like this. But this fix is not strictly backwards compatible...

We could log a warning when a for tag is used with reversed and a limit to see what this would break in Shopify.

[fw42]

There is an argument to be made here that

for item in array reversed limit:2

and

for item in array limit:2 reversed

should not behave the same way (one should return [3,2], the other should return [2,1])

[joost]

@fw42 The current syntax does not support the first (edit) second version you mention. See https://github.com/Shopify/liquid/blob/master/lib/liquid/tags/for.rb#L48

[fw42]

Good point :-)

Can you rebase this PR?

[pushrax]

In that case, since the syntax is for item in array limit:2 reversed, it arguably makes sense to have the current behaviour.

[joost]

true :)

[fw42]

Then maybe lets just implement the new syntax as well?

[pushrax]

👍, addresses the original issue and doesn't require verifying templates don't break.

[joost]

def test_for_reversed_with_limiting
  assigns = {'array' => [ 1, 2, 3] }
  assert_template_result('32','{%for item in array reversed limit:2 %}{{item}}{%endfor%}',assigns)
end

It is the other way around. Looking at the test I wrote for the PR (already some time ago :)) the behaviour previous to my PR was weird: [1,2,3] reversed limit:2 resulting in 21 instead of 32.

[pushrax]

So the syntax it supports is the first version in @fw42's earlier comment, not the second.

[joost]

Yep.

[fw42]

@pushrax: What should we do here? Add some logging to Shopify first to see if we can ship this without breakage? It's not backwards compatible.

[pushrax]

Could do this with static analysis – I need to get the harness set up for that again anyway (for the formatter and #569) so maybe that's the best course of action?

[fw42]

I need to get the harness set up for that again

@pushrax: Any intentions of still doing this?

[pushrax]

Not in the short term.

[jbrains]

I have no idea what it would mean to do this, but it seems that if you let for use filter-like behavior, then you could retain the old syntax for compatibility. Example:

for item in array reversed limit:2

Here, "reversed" and "limit:2" are effectively tags on the loop, so that keeps the current behavior. By comparison:

for item in (array | reverse | top:2)

filters the collection we iterate over, so the loop itself remains ignorant of the situation. This means adding a filter that takes the top n items of an array, which I'd like Liquid to add, anyway.

How feasible would this be, given the current design?

We could start with just adding a "top" filter, and in the worst case, we'd have

assign filtered = collection | reverse | top:2
for each in filtered ...

I'd be thrilled with that. Even a slice filter that operates on arrays and not just strings would help.

[stedman]

@pushrax @dylanahsmith I'm curious about the status of this PR. I would love to see the parameters execute as expected, from left to right (reversed then limit:2), but understand the desire to maintain backward compatibility.

As a compromise, would it be possible to optionally add a value to reversed to achieve the same goal? Current implementations would then be unaffected. For example:

# array = [1,2,3]
for i in array reversed limit:2           # [2,1] (default)
for i in array reversed:"first" limit:2   # [3,2] reverses collection first, then limits
for i in array reversed:"last" limit:2    # [2,1] limits first, then reverses collection (added for syntax consistency)