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

Use VariableLookup instead of dot notation in standard array filters #1899

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

karreiro
Copy link
Contributor

@karreiro karreiro commented Jan 17, 2025

Following @ianks' suggestion (https://github.com/Shopify/liquid/pull/1869/files#r1919321969), this PR updates standard array filters to use Liquid::VariableLookup instead of the dot notation convention.

I initially considered something similar to the usage of the Liquid::VariableLookup, but I noticed it would open the door to broader use cases like accessing arrays, which might not be ideal as mentioned here (#1869 (comment)).

However, in the interest of consistency, we see value in evaluating this approach, which is the goal of this PR :)

@jg-rp
Copy link
Contributor

jg-rp commented Jan 17, 2025

Parsing filter arguments as variables at render time, with or without VariableLookup.parse, has two potential downsides for me.

The first is one of efficiency. property arguments will be parsed at least once for every item in the input array. In the case of the sort filter, it could be that they will be parsed twice for each comparison (I don't actually know how Ruby's Array.sort works internally).

The second downside is regarding template static analysis. If we were walking a template's parse tree with ParseTreeVisitor, property arguments will always appear as strings and never instances of VariableLookup. Even if we were to implement some special case handling, knowing that some filters accept variables as strings, we'd have no way of knowing if those strings were intended to be a name or a path to a variable.

I understand that you might have already considered these things and decided that the current approach is still the best option. I'm just sharing my thoughts here in the absence of an RFC 😃.

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.

2 participants