-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 deep search for filter taking in properties #1749
base: main
Are you sure you want to change the base?
Conversation
@dylanahsmith I am not sure if you still work on Liquid, but I would love for someone in the Liquid team to take a look at this PR. |
@karreiro @benjaminsehl ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 Hey @andershagbard,
Thank you for this PR. Could you please share your reasoning behind the deep
parameter?
I feel like it was introduced with backward compatibility concerns; however, I believe it may bring a bit of complexity to the language and to language server implementations. I don't think it's required for backward compatibility, but I'm not sure if I'm missing something in that aspect.
Thanks again for this PR!
It's to safeguard against the JSON field type, which can contain dots in it's properties. It's an edge case I guess. {
"foo.bar": true
} Ref: #1436 (comment) |
This PR allows for backward compatibility deep search :woah:
Like so:
Added deep search for the following filters. This should be all Liquid filters which takes in a property argument: