code review 106360043: regexp: add ReplaceAll(String)?SubmatchFunc.

regexp: add ReplaceAll(String)?SubmatchFunc.

Fixes issue 5690.

[vmihailenco, 2014-07-01 11:44:36]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[rsc, 2014-07-09 16:14:44]

This is a very awkward API. Passing the submatches in separate function calls
and somehow pasting together the pieces means that you can't, for example, do
the equivalent of regexp.MustCompile(`(\w+) (\w+)`).ReplaceAllString(s, "$2
$1").

If we're going to add this method - and I am not sure it is worthwhile - I would
expect the function passed in to take a []string that is all the submatches.
Except for building the []string, the implementation is then a trivial call to
replaceAll.

[vmihailenco, 2014-07-10 08:51:43]

On 2014/07/09 16:14:44, rsc wrote:
> This is a very awkward API. Passing the submatches in separate function calls
> and somehow pasting together the pieces means that you can't, for example, do
> the equivalent of regexp.MustCompile(`(\w+) (\w+)`).ReplaceAllString(s, "$2
> $1").
> 
> If we're going to add this method - and I am not sure it is worthwhile - I
would
> expect the function passed in to take a []string that is all the submatches.
> Except for building the []string, the implementation is then a trivial call to
> replaceAll.

Thanks for review. I've updated code as you suggested. Working on this patch
I've found a workaround for my problem that does not require adding new method
to the regexp package. Actually it looks like all Replace* functions can be
implemented
on the user side using FindStringSubmatchIndex. So I don't insist on this
change.

[vmihailenco, 2014-07-10 08:52:25]

PTAL

[bradfitz, 2014-07-23 18:35:28]

https://codereview.appspot.com/106360043/diff/140001/src/pkg/regexp/regexp.go
File src/pkg/regexp/regexp.go (right):

https://codereview.appspot.com/106360043/diff/140001/src/pkg/regexp/regexp.go...
src/pkg/regexp/regexp.go:475: // ReplaceAllStringFunc returns a copy of src in
which all submatches of the
this doesn't match the method name.

https://codereview.appspot.com/106360043/diff/140001/src/pkg/regexp/regexp.go...
src/pkg/regexp/regexp.go:490: // ReplaceAllFunc returns a copy of src in which
all submatches of the
likewise

[vmihailenco., 2014-07-24 07:34:26]

On 2014/07/23 18:35:28, bradfitz wrote:
> https://codereview.appspot.com/106360043/diff/140001/src/pkg/regexp/regexp.go
> File src/pkg/regexp/regexp.go (right):
> 
>
https://codereview.appspot.com/106360043/diff/140001/src/pkg/regexp/regexp.go...
> src/pkg/regexp/regexp.go:475: // ReplaceAllStringFunc returns a copy of src in
> which all submatches of the
> this doesn't match the method name.
> 
>
https://codereview.appspot.com/106360043/diff/140001/src/pkg/regexp/regexp.go...
> src/pkg/regexp/regexp.go:490: // ReplaceAllFunc returns a copy of src in which
> all submatches of the
> likewise

Fixed.

PTAL

[rsc, 2014-07-24 13:55:06]

R=r

I am not convinced this API needs to be expanded. Rob designed the API, so I
will leave it for him.

[r, 2014-07-28 21:21:29]

NOT LGTM

This is ugly and hard to use. I don't think it carries enough weight. I had to
read the comments and the code several times to understand what was going on. 

The problem may exist but it's too rare an issue to deserve such obscure code
and warty API.

[mzanona, 2014-09-11 12:50:34]

On 2014/07/28 21:21:29, r wrote:
> NOT LGTM
> 
> This is ugly and hard to use. I don't think it carries enough weight. I had to
> read the comments and the code several times to understand what was going on. 
> 
> The problem may exist but it's too rare an issue to deserve such obscure code
> and warty API.

I am not sure if that is too rare. I am coming from Javascript and I am really
used to perform regexp replacing groups inside functions. This gives me complete
control over multiple case scenarios.

    /(\w+)\s(\w+)/.replace(function (match, group1, group2) { … }

It may look silly but depending on the rules you set it is possible to parse
complex documents with ease. Please if you can consider something that has an
approximation of this feature it would be great.

Thank you

[mzanona, 2014-09-11 12:52:43]

On 2014/09/11 12:50:34, mzanona wrote:
> On 2014/07/28 21:21:29, r wrote:
> > NOT LGTM
> > 
> > This is ugly and hard to use. I don't think it carries enough weight. I had
to
> > read the comments and the code several times to understand what was going
on. 
> > 
> > The problem may exist but it's too rare an issue to deserve such obscure
code
> > and warty API.
> 
> I am not sure if that is too rare. I am coming from Javascript and I am really
> used to perform regexp replacing groups inside functions. This gives me
complete
> control over multiple case scenarios.
> 
>     /(\w+)\s(\w+)/.replace(function (match, group1, group2) { … }
> 
> It may look silly but depending on the rules you set it is possible to parse
> complex documents with ease. Please if you can consider something that has an
> approximation of this feature it would be great.
> 
> Thank you

patching the code above:

    “hello world".replace(/(\w+)\s(\w+)/, function (match, word1, word2) { … })

[rsc, 2014-12-04 16:35:33]

R=close