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

Fix: respect parameter order when using "for ... reversed" #231

Merged
merged 8 commits into from
Sep 30, 2021

Conversation

stedman
Copy link
Contributor

@stedman stedman commented Jun 6, 2020

With regards to the for tag's reversed parameter, I found the current behavior of reversing a collection after all the other parameters execute a bit unexpected (and not very useful).

Given {% for i in (1..8) reversed limit: 4 %}{{ i }}{% endfor %}, I would expect the params are executed from left to right. In other words, the collection is reversed first and then limited to four items with an output of 8765.

Instead I get 4321, which is what I would expect with {% for i in (1..8) limit: 4 reversed %}{{ i }}{% endfor %} where the collection is limited to the first four items before reversing.

My use case for this is reversing a large collection of blog posts (so they are listed as most recent first) and then sizing those results down to five posts for the home page.

This proposed change will impact existing code that uses reversed with additional params. If this PR seems too risky, perhaps adding a new parameter (flipped?) with the proposed functionality would do the trick.

@harttle
Copy link
Owner

harttle commented Jun 7, 2020

I'm afraid it's not conformant which means it'll break Jekyll sites and Shopify templates, though I absolutely like this point.

Try file an issue on https://github.com/Shopify/liquid instead.

@stedman
Copy link
Contributor Author

stedman commented Jun 7, 2020

@harttle thanks for the quick feedback! It looks like this was proposed 5+ years ago but fizzled out.

I added my voice to the issue with yet another suggestion to extend reversed with an optional value to achieve the same goal: {% for i in (1..8) reversed:"first" limit:4 %} outputs 8765. We'll see what happens.

BREAKING CHANGES:
- previously deprecated `getTemplate()` and `getTemplateSync()` not no longer supported
- `opts` no longer support dynamic set in `parseFile()`, `renderFile()` arguments
@harttle harttle changed the base branch from master to v10 July 26, 2021 08:54
@harttle harttle closed this Jul 26, 2021
@harttle harttle reopened this Jul 26, 2021
@harttle harttle changed the base branch from v10 to v9.26 September 30, 2021 12:40
@harttle harttle merged commit 98aa541 into harttle:v9.26 Sep 30, 2021
@harttle
Copy link
Owner

harttle commented Sep 30, 2021

Added orderedFilterParameters option to enable this feature and changed your last test case

{% for i in (1..8) offset:2 reversed limit:3 %}{{ i }}{% endfor %}

expected value from 543 to 876 because reversed should happen before limit.

And there's a strange behavior, which I consider it as a bug, in Shopify liquid that

{% for i in (1..8) offset:2 reversed %}{{ i }}{% endfor %}

yields 345678, but

{% for i in (1..8) reversed offset:2 %}{{ i }}{% endfor %}

yields 876543. We'll not follow this behavior even if orderedFilterParameters is set to false.

harttle added a commit that referenced this pull request Sep 30, 2021
* fix: spelling

* fix: respect param order for reversed

* docs: add for-reversed order details

* perf: improve performance by 4x by simplified parseFile

BREAKING CHANGES:
- previously deprecated `getTemplate()` and `getTemplateSync()` not no longer supported
- `opts` no longer support dynamic set in `parseFile()`, `renderFile()` arguments

* perf: parse filenames in parse() insteadof render()

* docs: update description of LiquidJS

Co-authored-by: harttle <yangjvn@126.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants