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

feat: add find_index, has, and reject filters #799

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

brunodccarvalho
Copy link
Contributor

Implement new collection filters find_index, reject and has to match ruby side. Fixes #798

@brunodccarvalho
Copy link
Contributor Author

So the first issue is an inconsistent behaviour between where and find filters when undefined is given as the expected value. The where filter will detect undefined and filter for truthy entries, whereas find simply treats undefined as the literal value to grep for. Of course expected is undefined when it is omitted.

# data = [{ name: "Adamy" }, { name: "Blake", age: 50 }], xyz is not defined
{{ data | where: "age", xyz | json }} => [Blake]
{{ data | find: "age", xyz | json }} => Adamy

Consider the ruby implementation of find

From my understanding the first behaviour is roughly the (new) ruby behaviour, and the second is the jekyll behaviour. I imagine this inconsistency should be addressed, as find sounds like it should be functionally equivalent to where | first. Here find is changed to follow the first behaviour like the new filters. Can also consider a system option or checking arguments.length

@brunodccarvalho
Copy link
Contributor Author

Ruby liquid detects null to search for truthy values, here we detect only undefined. I suspect this is not a big issue.

Now find and find_index are returning undefined on misses while ruby wants to return null. This was changed on #723. It becomes functionally different after one more step, like appending | json or something like this which outputs NaN or 0 depending on what find returns:

{% assign blake = data | find: "age", 30 -%}      # wrong age
{{ blake.age | times: 2 }}

@harttle
Copy link
Owner

harttle commented Feb 12, 2025

Yes I agree we need find and where to be consistent, and returning null/undefined be consistent with liquid, at least in a way that it behaves same with one more step. Not sure if there’s any other consequences but we can have a try.
Checking arguments length sounds more intuitive but we need be consistent with shopify first (same template same result). We can have a jekyllFind toggle latter if someone has concrete cases.

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.

Add find_index, has, reject array filters
2 participants