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

Add find, find_index, has, and reject filters to arrays #1869

Merged
merged 5 commits into from
Jan 14, 2025

Conversation

karreiro
Copy link
Contributor

@karreiro karreiro commented Dec 12, 2024

This PR implements the RFC#1864 and introduces support for the find, find_index, has, and reject filters into the Liquid Array API.

The new filters support multi-level access to drops, so they can access properties like this:

{% assign product = products | find: 'price.unit', 'BRL' %}

For the sake of consistency, multi-level access has been introduced for all related filters in the Array API: map, sort, sort_natural, sum, uniq, where, and compact.

--

Special thanks to @andershagbard who started this work on #1573 and #1749. I'm including those commits as part of this PR; while the implementation approach is slightly different, important work happened there, and it's valuable to have that context in the Liquid commit history.

Also, thank you to everyone who contributed with thoughts and suggestions in the RFC.

@karreiro
Copy link
Contributor Author

👋 Hey @charlespwd @Maaarcocr,

Could you please take a look at this PR?

Thank you!

@karreiro karreiro requested a review from charlespwd January 13, 2025 16:32
Copy link
Contributor

@charlespwd charlespwd left a comment

Choose a reason for hiding this comment

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

LGTM :)

@karreiro karreiro merged commit 6909570 into main Jan 14, 2025
11 checks passed
@bakura10
Copy link

Awesome news <3. That's a very welcome addition to Liquid which will help us to clean up a lot of our code. Do you know when this will be available to use in Shopify?


keys = property_or_keys.split('.')
keys.reduce(drop) do |drop, key|
drop.respond_to?(:[]) ? drop[key] : drop
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems weird to me... does this mean that:

product.dafsdfasdfasdf #=> ProductDrop

I would have expected it to be like:

product.dafsdfasdfasdf #=> nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be weird indeed. But I believe it's already working as expected (and product.dafsdfasdfasdf already returns nil), as this test passes:

def test_prop_test
  products = [
    { "title" => "Pro goggles" },
    { "title" => "Thermal gloves", "price" => 1499 },
    { "title" => "Alpine jacket",  "price" => 3999 },
    { "title" => "Mountain boots", "price" => nil },
    { "title" => "Safety helmet",  "price" => 1999 },
  ]

  template = <<~LIQUID
    {%- assign example1 = products | find_index: 'dafsdfasdfasdf' -%}
    {{- example1 -}}
    {%- assign example2 = products | find_index: 'dafsdfasdfasdf.dafsdfasdfasdf' -%}
    {{- example2 -}}
  LIQUID
  expected_output = ""

  assert_template_result(expected_output, template, { "products" => products })
end

The reason we're achieving the expected behavior is that:

  • For example1, we're resolving that at line 985
  • For example2, we're resolving that because in the first iteration drop is product and returns nil for the first dafsdfasdfasdf call, then in the second call drop is already nil

So, I believe we're already achieving that behavior, but please let me know if I'm missing something :)

end
end

def fetch_property(drop, property_or_keys)
Copy link
Contributor

@ianks ianks Jan 16, 2025

Choose a reason for hiding this comment

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

I'm not sure how I feel about re-implementing lookup logic here. There's already a way to do that which is well-defined with VariableLookup.parse. It even has built-in caching to mitigate performance issues of repeated parsing. Could we use this instead?

irb(main):006> Liquid::VariableLookup.parse("product.images[0]")
=> #<Liquid::VariableLookup:0x000000010565ce98 @command_flags=0, @lookups=["images", 0], @name="product">

Copy link
Contributor

Choose a reason for hiding this comment

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

I have reservations about parsing strings at render time in general. At the very least it will play havoc with static analysis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's continue this thread here:
#1899 :)

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.

7 participants