Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: regexp: add Replace and ReplaceString #40198

Closed
kpym opened this issue Jul 14, 2020 · 17 comments
Closed

proposal: regexp: add Replace and ReplaceString #40198

kpym opened this issue Jul 14, 2020 · 17 comments

Comments

@kpym
Copy link

kpym commented Jul 14, 2020

What version of Go are you using (go version)?

1.14

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

Any

What did you do?

I try to replace not all occurrences using regexp library

What did you expect to see?

To have Replace and ReplaceString methods that take an extra integer parameter that indicat the maximal number of replacements, like in the python's re.subs.

Here is my proposal for ReplaceString:

func (re *Regexp) ReplaceString(src, repl string, n int) string

	var re = regexp.MustCompile(re)

	src = re.ReplaceAllStringFunc(src, func(s string) string {
		if n == 0 {
			return s
		}

		n -= 1
		return re.ReplaceAllString(s, repl)
	})

	return src
}
@gopherbot gopherbot added this to the Proposal milestone Jul 14, 2020
@ianlancetaylor ianlancetaylor changed the title Proposal: add Replace and ReplaceString in regexp proposal: regexp: add Replace and ReplaceString Jul 14, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jul 14, 2020
@fplanque

This comment has been minimized.

@rsc
Copy link
Contributor

rsc commented Aug 28, 2020

You're talking about adding six more methods to regexp.Regexp, which is already drowning in methods.
Especially since you can implement this easily with the closure (nice observation!),
I'm not sure it needs to be added to the standard library.

Is there any strong evidence that this sort of thing is needed?
(Examples: popular Go packages providing this as an add-on, evidence of widespread use in other languages, ...)

@ianlancetaylor

This comment has been minimized.

@kpym
Copy link
Author

kpym commented Aug 29, 2020

@rsc Yes it is easy to implement, like most of the "basic" functions. This can be seen as argument in both directions to add or to not add to the standard library.

My main argument is that this is a basic functionality present in most of the languages precisely because it is used very often.
My second argument is that the implementation is not a "one line code", even if it is easy to implement.

But I don't have "strong evidence that this is needed". Maybe somebody else can give such evidence ...

@rsc rsc moved this from Incoming to Active in Proposals (old) Sep 18, 2020
@rsc
Copy link
Contributor

rsc commented Sep 23, 2020

Adding to the proposal minutes for visibility.
We'd need to see more need beyond what is already there.

@thepudds
Copy link
Contributor

Perhaps a solution to consider could be adding one example to the doc showing the basic approach?

That said, there are already many examples in the package, so discoverability and overall noise level might be a concern.

@ianlancetaylor
Copy link
Contributor

For code like this, this issue itself can serve as a way to address the problem. I'm not sure it has to be in the package docs.

@rsc
Copy link
Contributor

rsc commented Sep 30, 2020

Based on the discussion above, this seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Sep 30, 2020
@kpym
Copy link
Author

kpym commented Sep 30, 2020

@rsc ok. Thanks for considering my request anyway.

@fplanque
Copy link

Is there any strong evidence that this sort of thing is needed?
(Examples: popular Go packages providing this as an add-on, evidence of widespread use in other languages, ...)

I use the limit param in PHP all the time: https://www.php.net/manual/en/function.preg-replace.php

I am annoyed that I cannot use it in Go.

@rsc
Copy link
Contributor

rsc commented Oct 1, 2020

PHP contains much that Go does not.

(Also, if you are annoyed about needing this limit in Go frequently, I would suggest writing a function along the lines of the one at the top of this issue, and then you won't need to be annoyed anymore.)

@fplanque
Copy link

fplanque commented Oct 1, 2020

I am just replying to "evidence of widespread use in other languages".
If you now answer "other languages have (much) more than Go", then I am not sure why you asked for evidence in the first place. (?)

Here is another example:
Python also has an easy and optional count argument for max number of replacements.
https://docs.python.org/3/library/re.html#module-contents

@moorereason
Copy link

I can only report that we offer a template function in Hugo that supports a regexp replace with a maximum. We used the closure mentioned at the top of this issue.

I understand the argument against added a half-dozen functions to the package. So, I offer a compromise: change the ReplaceAll* function signatures in Go2 to add a n int parameter. The FindAll* functions already have a n parameter. It seems odd that the ReplaceAll* functions didn't follow suit. Can we fix this in Go2?

@ianlancetaylor
Copy link
Contributor

In effect there is no Go 2, there is just the possibility of regexp/v2. But that doesn't seem all that likely, as the regexp package is working fine.

Still, examples of cases where people would use limits are helpful. It might help to point to some places where people do use a limit, and why. That is, it does help to show other languages and frameworks that provide a limit. But it helps even more to show where people use that limit in their code. Thanks.

@fplanque
Copy link

fplanque commented Oct 1, 2020

Use case example: in Hugo, we want to find to replace the first occurence of each glossary term on a given web page with a link to the definition in the Glossary. We do not want to link 100 occurrences of a word or acronym in an article.

@kpym
Copy link
Author

kpym commented Oct 1, 2020

My initial use case was similar to @fplanque's case : I wanted to replace only the first occurance in a program charged to convert from one file format to another.

@rsc
Copy link
Contributor

rsc commented Oct 7, 2020

Maybe at some point we should revisit the regexp API, but I don't think this incremental addition pulls its weight especially given the Func trick.

In retrospect the All form should have taken an n, like FindAll etc, but it's too late for that now.

No change in consensus, so declined.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

7 participants