-
Notifications
You must be signed in to change notification settings - Fork 68
PR #42 filter parameters (breaking change) #131
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
base: main
Are you sure you want to change the base?
Conversation
abc829c to
4892437
Compare
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.
Pull request overview
This PR implements named filter arguments (issue #42), enabling Shopify Liquid-compatible filter parameter syntax. The implementation adds support for calling filters with named parameters like {{image | img_url: '580x', scale: 2}} and {{ date | format: 'date' }}.
- Introduced
filterParamstruct to distinguish between positional and named parameters - Updated filter application logic to separate and handle both parameter types
- Modified parser grammar to recognize named argument syntax (
KEYWORD expr) - Added comprehensive test coverage for various named argument scenarios
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| expressions/filters.go | Core implementation: added filterParam struct and updated ApplyFilter to handle named arguments via map parameter detection |
| expressions/expressions.y | Grammar changes: extended filter_params rule to support KEYWORD expr syntax for named parameters |
| expressions/y.go | Regenerated parser from grammar changes (goyacc output) |
| expressions/scanner.go | Regenerated scanner with formatting changes (ragel output) |
| expressions/context.go | Updated ApplyFilter interface signature to accept []filterParam instead of []valueFn |
| expressions/builders.go | Updated makeFilter function signature to match new parameter type |
| expressions/filters_test.go | Added comprehensive unit tests for named arguments, including parsing and various argument combinations |
| liquid_test.go | Added integration tests demonstrating real-world usage from issue #42 examples |
| README.md | Added detailed documentation with examples of named filter arguments; removed issue #42 from unimplemented features list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
expressions/filters.go
Outdated
| } | ||
|
|
||
| // Add named arguments map if the filter accepts them | ||
| if acceptsNamedArgs && len(namedParams) > 0 { |
Copilot
AI
Nov 29, 2025
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.
The named arguments map is only added when len(namedParams) > 0, but filters expecting named arguments should always receive a map parameter even when no named arguments are provided. This breaks filters that rely on checking the map for optional arguments.
For example, when calling {{image | img_url: '300x'}} (no named args), the filter function func(image string, size string, opts map[string]any) will not receive the opts parameter at all, causing a parameter count mismatch error.
Solution: Always pass the namedParams map if the filter accepts named arguments, even when it's empty:
if acceptsNamedArgs {
args = append(args, namedParams)
}| if acceptsNamedArgs && len(namedParams) > 0 { | |
| if acceptsNamedArgs { |
| //line scanner.rl:1 | ||
| package expressions |
Copilot
AI
Nov 29, 2025
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.
The header comment // Code generated by ragel. DO NOT EDIT. has been removed. According to the //go:generate directive in expressions/parser.go, this comment should be added automatically by sed after ragel generation. This file should not be manually edited.
Additionally, there are numerous whitespace-only changes (removal of blank lines) throughout the file that appear to be formatting changes, which suggests the file may have been manually edited or regenerated with different formatting settings.
6b269a3 to
b5c0653
Compare
Implement filter parameters for Shopify Liquid compatibility.
Filters can now accept named arguments alongside positional args.
Examples:
{{image | img_url: '580x', scale: 2}}
{{ order.created_at | date: format: 'date' }}
{{ 'customer.order.title' | t: name: order.name }}
Filter functions receive named args as map[string]any in their last parameter.
BREAKING CHANGE: The expressions.Context interface signature changed.
External implementations of ApplyFilter must update their signature from
ApplyFilter(string, valueFn, []valueFn) (any, error)
to
ApplyFilter(string, valueFn, []filterParam) (any, error)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
b5c0653 to
aab7d0b
Compare
Implement filter parameters.