Add Regex::MatchData#captures, named_captures, to_a, to_h

[makenowjust]

to_a and to_h are just converting methods, so they follow such semantics: self[idx] == self.to_a[idx] and self[idx] == self.to_h[idx] && self[key] == self.to_h[key].

But captures and named_captures are not. captures contains only unnamed captures, and named_captures contains only named captures. Those methods look like Ruby.

[Sija]

Ping? Any chances on getting it merged before next release? 🎉

[makenowjust]

Thanks @Sija. I rebased this branch to resolve conflict for the first step to merge.

[Sija]

@crystal-lang lets get this merged, please :)

[Sija]

@asterite Is there anything blocking this PR from merging? These are useful additions (as a plus bringing more feature-parity with Ruby).

[Sija]

@asterite ping?

[Sija]

Hi, is there anything preventing this PR from being merged? Almost half-year passed since its inception with completely no response from the core team... Bad PR karma :/

[makenowjust]

I didn't intend this, however it was counter PR against #3653. So I should explain pros/cons of this PR.

Pros:

1. It has simple principles such explained in above comment: self[idx] == self.to_a[idx] and self[idx] == self.to_h[idx] && self[key] == self.to_h[key]. It is useful to pass MatchData as Array or Hash to a restricted method. I think Enhancement to Regex::MatchData methods #3653's to_a and to_h are too complex and I don't know when such a behavior is useful. (NOTE: Both behaviors are different from Ruby.)
2. Its implementations are memory efficient enough. (Maybe, if you found my wrong, I'd be welcome your report.)

Cons:

1. Its spec is poorer than Enhancement to Regex::MatchData methods #3653's. Should I write more specs?

[mverzilli]

Hey, sorry for the delay. What's preventing this from getting merged is that we need to compare it with #3653 and review the implementation to ensure it doesn't introduce performance issues, it's well documented etc.

[makenowjust]

@mverzilli What did you find an issue?

[Sija]

Bump @mverzilli @asterite

[asterite]

self[i] should be self[i]?, the other PR might be better

[Sija]

@asterite Wouldn't (0..size) guarantee existence of given i index?

[asterite]

@Sija No:

# Ruby
m = "foo".match(/(f)(x)?/)
p m.captures # => ["f", nil]
p m.to_a # => ["f", "f", nil]

m = "foo".match(/(f)(?<name>x)?/)
p m.named_captures # => {"name"=>nil}
p m.to_a # => ["f", nil]

Optional groups are in the MatchData as nil, and you get them with []?. Doing it with [] raises. With this PR if I try this:

m = "foo".match(/(f)(x)?/).not_nil!
p m.captures
p m.to_a

m = "foo".match(/(f)(?<name>x)?/).not_nil!
p m.named_captures
p m.to_a

Both captures and named_captures raise an exception, when they should never raise. Using self[i]? gives exactly the same result as Ruby.

I'll probably implement this myself soon, don't worry :-)

[makenowjust]

@asterite I see. I didn't consider this case. My note PC is in my house today. I'd like you to implement it for fast merge. Thank you!

[Sija]

@asterite Of course, totally forgot about optional groups... thanks for reminder! :) Regarding this PR vs the other one, personally I prefer this one due to familiar API (Ruby), just my 2¢.

[asterite]

@makenowjust OK, I'll wait. Take your time, there's no rush at all. Thank you!

[makenowjust]

@asterite @Sija Updated this PR 😄

[asterite]

@makenowjust Excellent, thank you! 🙇