Implement String#split with block #3702

splattael · 2016-12-15T14:57:32Z

Hi,

this PR enhances all flavors of String#split with a block.

After this PR, you can pass a block to String#split:

"foo bar".split { |chunk| p chunk }
"foo bar".split(' ') { |chunk| p chunk }
"foo bar".split(" ") { |chunk| p chunk }
"foo bar".split(/ /) { |chunk| p chunk }

Additional tests were not added because all non-block versions of String#split exercise String#split(..., &block). (Only one test for an edge case was added, though)

Kind regards,
Peter

kostya · 2016-12-15T15:12:25Z

#2541

splattael · 2016-12-15T15:15:34Z

@kostya Dammit, should have searched for it before ;)

Sorry, for the noise.

(I am actually using the block version to speed-up things...)

kostya · 2016-12-15T15:17:30Z

it was not merged, and i also want a version which yield slice, for speed

splattael · 2016-12-15T15:21:05Z

@kostya Yeah, my version yields Strings which is slower than Slice but still faster than "foo bar".split(' ').each { ... }.

Anyhow, if it's not wanted, I'll keep my monkey patch^W^Wextension for now ;)

splattael · 2016-12-15T15:24:00Z

spec/std/string_spec.cr

@@ -738,6 +738,7 @@ describe "String" do
      assert { "   foo  bar".split(' ').should eq(["", "", "", "foo", "", "bar"]) }
      assert { "   foo   bar\n\t  baz   ".split(' ').should eq(["", "", "", "foo", "", "", "bar\n\t", "", "baz", "", "", ""]) }
      assert { "   foo   bar\n\t  baz   ".split.should eq(["foo", "bar", "baz"]) }
+      assert { "   foo   bar\n\t  baz   ".split(1).should eq(["   foo   bar\n\t  baz   "]) }


@asterite Even if #3702 won't be merged maybe you could 🍒 this test?

ysbaddaden · 2016-12-15T15:30:20Z

I'm reopening the PR. The problem of #2541 is avoided because it yields String instead of an unexpected Slice —which would fit better as Slice#split and be just as awesome. Maybe it could implement Slice#split(Char) and use it, but that wouldn't cover other cases (String, Regex), and I think this is a great improvement already (that could be further improved).

String 👍 from me!

kostya · 2016-12-15T15:32:02Z

yielding slice is ok, because it internal method which i need to use for speed, common method still return strings.

asterite · 2016-12-15T15:35:32Z

I'm also OK with this addition.

Eventually I'd also like to be able to get an Iterator from a split. Maybe it can be named each_split.

@kostya We could add a separate method that yields slices, with a different name (maybe each_split_slice or something like that). Slices are much more low-level and needed when you want to squeeze performance in case of a bottleneck, and Crystal should allow you do do that, but the default is usually "user-friendly" (String is better than Slice, Array is better than Slice and StaticArray, etc.)

asterite · 2016-12-20T11:49:20Z

src/string.cr

+  #   water's sound
+  # "
+  # old_pond.split { |s| ary << s }; ary    # => ["Old", "pond", "a", "frog", "leaps", "in", "water's", "sound"]
+  # old_pond.split(3) { |s| ary << s }; ary # => ["Old", "pond", "a frog leaps in\n  water's sound\n"]


ary should probably be cleared before each split. This applies to this example and other examples below.

@asterite Good catch! Fixed.

splattael · 2016-12-22T15:06:01Z

@asterite Ready to merge. Do you want me to squash the commits?

asterite · 2016-12-22T15:11:10Z

@splattael No problem, I think the separate commits look good :-)

I'll merge it soon. Thank you!

splattael closed this Dec 15, 2016

splattael commented Dec 15, 2016

View reviewed changes

ysbaddaden reopened this Dec 15, 2016

asterite reviewed Dec 20, 2016

View reviewed changes

Peter Suschlik added 5 commits December 20, 2016 16:15

Implement String#split(limit, &blk)

7a7904e

Implement String#split(Char, limit, &blk)

d6317b6

Implement String#split(String, limit, &blk)

fb24286

Implement String#split(Regex, limit, &blk)

69c4f93

Let String#split_by_empty_separator yield results

f268f8b

asterite added kind:feature topic:stdlib labels Dec 22, 2016

asterite added this to the 0.20.2 milestone Dec 22, 2016

asterite merged commit b8f0151 into crystal-lang:master Dec 22, 2016

bmulvihill mentioned this pull request Dec 23, 2016

Add String#each_split #3754

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement String#split with block #3702

Implement String#split with block #3702

splattael commented Dec 15, 2016

kostya commented Dec 15, 2016

splattael commented Dec 15, 2016

kostya commented Dec 15, 2016

splattael commented Dec 15, 2016

splattael Dec 15, 2016

ysbaddaden commented Dec 15, 2016

kostya commented Dec 15, 2016

asterite commented Dec 15, 2016

asterite Dec 20, 2016 •

edited

splattael Dec 20, 2016

splattael commented Dec 22, 2016

asterite commented Dec 22, 2016

Implement String#split with block #3702

Implement String#split with block #3702

Conversation

splattael commented Dec 15, 2016

kostya commented Dec 15, 2016

splattael commented Dec 15, 2016

kostya commented Dec 15, 2016

splattael commented Dec 15, 2016

splattael Dec 15, 2016

Choose a reason for hiding this comment

ysbaddaden commented Dec 15, 2016

kostya commented Dec 15, 2016

asterite commented Dec 15, 2016

asterite Dec 20, 2016 • edited

Choose a reason for hiding this comment

splattael Dec 20, 2016

Choose a reason for hiding this comment

splattael commented Dec 22, 2016

asterite commented Dec 22, 2016

asterite Dec 20, 2016 •

edited