Skip to content

Conversation

@vanstee
Copy link
Contributor

@vanstee vanstee commented Aug 26, 2013

To support the trim option I had to reimplement split to remove all empty strings from the result.

Here's an example:

iex> String.split(" a b c ", " ", trim: false)
["", "a", "b", "c", ""]
iex> String.split(" a b c ", " ", trim: true)
["a", "b", "c"]

…-repeats"

This reverts commit fb30101, reversing
changes made to c348eb7.
@rafacv
Copy link

rafacv commented Aug 26, 2013

Hi @vanstee,
String.split has a very strict purpose; IMO adding trim option to it would not only add complexity as also blur its definition. I'd rather implement String.trim to use in support of String.split to achieve the desired outcome.

Going beyond, I think global argument in String.split is pretty bland. A limit option that enables you to choose how many iterations to be performed would be much more useful. It's matter for another PR though.

Please, take into account that I'm completely unaware of any context to this Pull Request so my opinion might be wrong and I reserve my right to change my mind later. 😄

@josevalim
Copy link
Member

@vanstee Why did you have to reimplement it? Couldn't you simply filter the results afterwards? :)

@vanstee
Copy link
Contributor Author

vanstee commented Aug 26, 2013

@josevalim That's a way better idea. I think I got a little carried away :P

@josevalim
Copy link
Member

@vanstee ok, let me know when you change it and i will merge it right away :)

@vanstee
Copy link
Contributor Author

vanstee commented Aug 26, 2013

@rafacv Oh I should have linked to the previous issue for some more context: #1645

Since :binary.split supports trim, it seemed fitting to add it to String.split. I don't have a strong opinion about global but I know Regex.split has a parts option similar to your limit idea. I think both of these are copying the options available in Erlang.

By default `trim` will be set to true. Because `:binary.strip` only
removes empty strings from the end of the result, we have to apply a
filter to remove empty strings from the beginning before returning.
By default `trim` will be set to true. However, since the `global` option
was implemented with the `parts` option (`{ :parts, :infinity }`), we
have to apply a filter to the final result to support `trim`.
@vanstee
Copy link
Contributor Author

vanstee commented Aug 26, 2013

Ahh... So much nicer with less code :) Thanks for the suggestion @josevalim!

josevalim pushed a commit that referenced this pull request Aug 26, 2013
Add support for `trim` option to `String.split` and `Regex.split`
@josevalim josevalim merged commit 89bebea into elixir-lang:master Aug 26, 2013
@vanstee vanstee deleted the string-split-trim branch August 26, 2013 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants