Add an optional limit argument to split filter #1801
Conversation
|
Thanks for the PR! The typo in the PR title should be corrected to "Optional" I guess. @gooroodev, can you review this too please? |
ahangarha
changed the title
lib/liquid/standardfilters.rb
Outdated
| def split(input, pattern) | ||
| input.to_s.split(pattern.to_s) | ||
| def split(input, pattern, limit = nil) | ||
| limit = limit.respond_to?(:to_i) ? limit.to_i : 0 |
There was a problem hiding this comment.
I think we should use Liquid::Utils.to_integer here.
There was a problem hiding this comment.
Oh! I didn't know. I will update the PR ASAP.
There was a problem hiding this comment.
As per my research Liquid::Utils.to_integer might not be helpful since it doesn't return 0 if an empty string gets passed. Not sure if I am too sensitive here or the 'to_integer' method needs some update.
What do you suggest? @ianks
There was a problem hiding this comment.
Sorry I meant Liquid::Utils.to_number
There was a problem hiding this comment.
I made the requested change. Also I updated the method docs.
There was a problem hiding this comment.
I wonder if it is necessary to ensure any input for limit gets converted to integer.
I think using to_integer is good enough and it throws a good error.
I raise this issue here because it might be better to be more strict at first and open up if future. Otherwise, it becomes a breaking change if we want to strict it later.
ahangarha
force-pushed
the
add-limit-arg-to-split-filter
branch
from
f206f7e
to
ffcfa27
Compare
This PR adds an optional limit argument to the
splitfilter in Liquid to make it work similar to the Rubysplitmethod.Closes #1800