Enhance slice filter with Unicode support and array slicing (#72) #126
Conversation
- Add proper Unicode handling using runes instead of regex - Fix performance issues with strings longer than 1000 characters - Add support for slicing arrays/slices, not just strings - Improve bounds checking: clamp out-of-bounds negative indices to 0 - Add comprehensive test coverage: - Unicode strings (Japanese characters) - Very long strings (10,000+ characters) - Out-of-bounds indices (positive and negative) - Array slicing from split operations Co-authored-by: Olivier Favre <olivier@wonderpush.com> Resolves #72
osteele
deleted the
claude/rebase-review-liquid-pr-011CUrSNw4Gv4suTCJNi3erS
branch
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the slice filter to add proper Unicode support, improve performance with long strings, and extend functionality to support array/slice slicing in addition to string slicing.
Key changes:
- Replaced string-specific implementation with a generic interface{}-based approach that handles strings, byte arrays, and slices
- Added proper Unicode handling by using runes instead of byte indices for string slicing
- Improved bounds checking by clamping out-of-bounds negative indices to 0 instead of returning empty results
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| filters/standard_filters.go | Refactored slice filter to accept interface{} instead of string, added support for slicing arrays/slices using values.Convert, improved negative index bounds checking to clamp to 0 |
| filters/standard_filters_test.go | Added comprehensive test coverage for Unicode strings (Japanese characters), very long strings (10,000+ characters), out-of-bounds indices (both positive and negative), and array slicing from split operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if start < 0 { | ||
| start = len(runes) + start | ||
| if start < 0 { | ||
| start = 0 | ||
| } | ||
| } | ||
| if start > len(runes) { | ||
| start = len(runes) | ||
| } | ||
| end := start + n | ||
| if end > len(runes) { | ||
| end = len(runes) | ||
| } | ||
| return string(runes[start:end]) | ||
| } | ||
|
|
||
| end := start + n | ||
| if end > len(ss) { | ||
| end = len(ss) | ||
| // Are we in the slice case? | ||
| // A type test cannot suffice because []T and []U are different types, so we must use conversion. | ||
| var slice []interface{} | ||
| if sliceIface, err := values.Convert(v, reflect.TypeOf(slice)); err == nil { | ||
| var ok bool | ||
| if slice, ok = sliceIface.([]interface{}); ok { | ||
| n := length(1) | ||
| if start < 0 { | ||
| start = len(slice) + start | ||
| if start < 0 { | ||
| start = 0 | ||
| } | ||
| } | ||
| if start > len(slice) { | ||
| start = len(slice) | ||
| } | ||
| end := start + n | ||
| if end > len(slice) { | ||
| end = len(slice) | ||
| } | ||
| return slice[start:end] |
There was a problem hiding this comment.
The bounds checking logic (lines 207-220 for strings and 229-242 for slices) is duplicated. Consider extracting this into a helper function to reduce code duplication and improve maintainability. For example:
func clampSliceBounds(start, length, size int) (int, int) {
if start < 0 {
start = size + start
if start < 0 {
start = 0
}
}
if start > size {
start = size
}
end := start + length
if end > size {
end = size
}
return start, end
}This would make the code more maintainable and reduce the chance of inconsistencies between the two implementations.
|
|
||
| if start < 0 { | ||
| start = len(ss) + start | ||
| fd.AddFilter("slice", func(v interface{}, start int, length func(int) int) interface{} { |
There was a problem hiding this comment.
The code uses interface{} instead of any, which is inconsistent with the rest of the codebase. All other filters in this file use any (e.g., lines 31, 38, 71, 78, 111, 290, 298). Consider using any for consistency:
fd.AddFilter("slice", func(v any, start int, length func(int) int) any {| fd.AddFilter("slice", func(v interface{}, start int, length func(int) int) interface{} { | |
| fd.AddFilter("slice", func(v any, start int, length func(int) int) any { |
| var slice []interface{} | ||
| if sliceIface, err := values.Convert(v, reflect.TypeOf(slice)); err == nil { | ||
| var ok bool | ||
| if slice, ok = sliceIface.([]interface{}); ok { |
There was a problem hiding this comment.
The code uses []interface{} instead of []any, which is inconsistent with the rest of the codebase. All other filters in this file use []any (e.g., lines 44, 53, 58, 71, 78, 303). Consider using []any for consistency:
var slice []any
if sliceIface, err := values.Convert(v, reflect.TypeOf(slice)); err == nil {
var ok bool
if slice, ok = sliceIface.([]any); ok {| var slice []interface{} | |
| if sliceIface, err := values.Convert(v, reflect.TypeOf(slice)); err == nil { | |
| var ok bool | |
| if slice, ok = sliceIface.([]interface{}); ok { | |
| var slice []any | |
| if sliceIface, err := values.Convert(v, reflect.TypeOf(slice)); err == nil { | |
| var ok bool | |
| if slice, ok = sliceIface.([]any); ok { |
Resolves #72
Checklist
make testpasses.make lintpasses.