Fix slice #72
Fix slice #72
Conversation
These tests currently pass, so I'm committing them alone I intend on changing the implementation and need to ensure they stay green.
The previous implementation compiled a regex on the fly, using a repetition count that could go above the supported range, causing it to panic. This implementation should work on any string size, offset and length. I added edge case tests when using out-of-bounds values. I do not know what Shopify implementation would return, and the documentation does not specify any behavior, so I came up with a sensible behavior.
Shopify's Liquid documentation says it works on arrays. This was not covered by the previous implementations.
|
Hi! Mind rebasing this? :) |
|
@ofavre ping ☝🏻 |
|
Hi @ofavre Thank you for this excellent contribution! I've reviewed the changes and the implementation looks great. The slice filter enhancements are well-designed and have great test coverage. This PR currently has conflicts with the main branch due to PR #115 that I accidentally merged after your branch was created (I meant to go through these in order), and it now needs to be rebased; or, I could replace it by another branch that I've prepared. I've created a successfully rebased version at claude/rebase-review-liquid-pr-011CUrSNw4Gv4suTCJNi3erS that you can reference. The conflicts were:
All tests pass successfully after the rebase. You have two options: Option 1: I can create a new PR from the rebased branchThis is the quickest path to getting your changes merged. I'll close this PR and create a new one. Your authorship will be fully preserved—you'll still be credited as the author of all three commits, and it will appear in your GitHub contribution history. Option 2: You can rebase your branchIf you prefer to keep this PR: # Update your local main branch
git checkout main
git pull upstream main
# Rebase your feature branch
git checkout <your-branch-name>
git rebase main
# Resolve conflicts in filters/standard_filters.go and filters/standard_filters_test.go
# Reference: https://github.com/osteele/liquid/tree/claude/rebase-review-liquid-pr-011CUrSNw4Gv4suTCJNi3erS
# Continue the rebase
git add filters/standard_filters.go filters/standard_filters_test.go
git rebase --continue
# Force push to update the PR
git push --force-with-lease origin <your-branch-name>Either way works! Let me know what you prefer. Great work on this feature! |
- 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#126) - 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 Resolves osteele#72 Co-authored-by: Olivier Favre <olivier@wonderpush.com>
3 participants
slicewas broken with an offset or length above 1000.Shopify documents that
slicesupports arrays in addition to strings; I added support for working on slices.Checklist
make testpasses.make lintpasses. (at least on modified files)