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

bytes, strings: add ReplaceAll #27864

Closed
HaraldNordgren opened this issue Sep 25, 2018 · 23 comments
Closed

bytes, strings: add ReplaceAll #27864

HaraldNordgren opened this issue Sep 25, 2018 · 23 comments
Labels
FeatureRequest FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@HaraldNordgren
Copy link
Member

Please answer these questions before submitting your issue. Thanks!

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

Does this issue reproduce with the latest release?

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

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

What did you expect to see?

What did you see instead?

@cznic
Copy link
Contributor

cznic commented Sep 25, 2018

That would be strings.Replace(s, old, new, -1)

@HaraldNordgren
Copy link
Member Author

Thanks for the downvote. I created a PR here https://go-review.googlesource.com/c/go/+/137456 and the implementation is exactly to call strings.Replace(s, old, new, -1).

My problem with the current state of affairs is that I find myself looking up the syntax of this every time I use it. Or if I am reviewing someone else's code I stumble over the -1 because I forget why it's there. So in the spirit of keeping Go simple I want to introduce this helper.

@bcmills bcmills added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. FeatureRequest labels Sep 25, 2018
@bcmills bcmills added this to the Go1.12 milestone Sep 25, 2018
@bradfitz
Copy link
Contributor

Numbers would be interesting here.

What percentage of strings.Replace calls in the wild hard-code -1?

@cznic
Copy link
Contributor

cznic commented Sep 25, 2018

> What percentage of strings.Replace calls in the wild hard-code -1?

I guestimate that on the small sample of my own code it's about 99%. Yet I don't think a function that just sets a default value to one of the parameters of another stdlib function belongs to the stdlib.

@HaraldNordgren
Copy link
Member Author

HaraldNordgren commented Sep 25, 2018

@bradfitz From my company's codebase, strings.Replace is called 12 times and all but one of them uses -1 as n. That also includes helper functions that are called in lots of places, so the total ratio shifts higher toward almost exclusively the 'ReplaceAll' behavior.

@HaraldNordgren
Copy link
Member Author

@cznic Btw, there are these helper in the standard library already:

from powser2:

func Print(U PS) {
	Printn(U, 1000000000)
}

from draw:

func Draw(dst Image, r image.Rectangle, src image.Image, sp image.Point, op Op) {
	DrawMask(dst, r, src, sp, nil, image.Point{}, op)
}

from ast:

func Inspect(node Node, f func(Node) bool) {
	Walk(inspector(f), node)
}

@magical
Copy link
Contributor

magical commented Sep 25, 2018

@HaraldNordgren powser2.go is a test, not library code. draw.Draw seems like a good example though.

@bradfitz
Copy link
Contributor

Or strings.Fields and strings.FieldFunc.

Or http.ListenAndServe vs http.Server.ListenAndServe.

Or regexp.Match vs regexp.Compile+Match.

Or regexp.MustCompile vs regexp.Compile, or template.Parse vs template.MustParse.

I don't agree with @cznic thumbs down for this; the standard library is full of convenient-vs-lower-level versions of things.

I'd rather see numbers.

@jimmyfrasche
Copy link
Member

in my personal/work code: 16 calls, all -1.

@bradfitz
Copy link
Contributor

In Perkeep, 60 use -1 and 43 use 1. No other values.

@dpinela
Copy link
Contributor

dpinela commented Sep 25, 2018

There are 265 calls to strings.Replace in the main Go repo, of which 216 use -1.

@dominikh
Copy link
Member

In the Go corpus, excluding vendored code, 1034 of 2213 calls to strings.Replace hard-code -1.

I'm with cznic on this one. Nearly all of the examples provided by bradfitz are more complex (syntactically and semantically) than providing a trivial default value for an argument. Panicking instead of returning an error is a significant change in API. regexp.Match encapsulates two separate operations. strings.Field is far more complex, providing an optimized implementation and pulling in additional dependencies.

In all of these cases, there's really no question as to which function should be used. I would rather not, however, tell people to use ReplaceAll instead of Replace, and I wouldn't want to see a mix of old and new code, either.

@bradfitz
Copy link
Contributor

and I wouldn't want to see a mix of old and new code, either.

That's somewhat inevitable with the standard library's policy of never removing old stuff or mistakes. e.g. we have a mix of Seek values 0 vs os.SEEK_SET vs io.SeekStart and it's not terrible, even if it's not ideal.

@gregory-m
Copy link
Contributor

Excluding generated and vendored code.

In k8s codebase.
100 use -1, 14 use 1. No other values used.

in moby code base:
30 use -1, 6 use 1, No other values used.

@rsc
Copy link
Contributor

rsc commented Sep 26, 2018

regexp.Regexp has a ReplaceAll, so the name is at least with precedent. If we had it to do over again I'd make Replace like Split, where the default is "all" and we could have ReplaceN for a limited count. But we don't have it to do over again. So ReplaceAll seems OK. Should be done to both strings and bytes though.

@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 26, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 26, 2018
@rsc rsc changed the title strings: Create 'ReplaceAll' convenience function bytes, strings: add ReplaceAll Sep 26, 2018
@cznic
Copy link
Contributor

cznic commented Sep 26, 2018

Replace(s, old, new, -1)
ReplaceAll(s, old, new)

One character saved.

One minute wasted for a Go programmer to lookup the new API function documentation.

Times ~1 million of them.

@dsnet
Copy link
Member

dsnet commented Sep 26, 2018

FYI. At Google, 65% of all {strings,bytes}.Replace usages hardcode -1.

Edit: looks like I'm late to the party and this is already accepted. Ignore me.
Edit2: s/hardcore/hardcode/... haha.

@bradfitz
Copy link
Contributor

FYI. At Google, 65% of all {strings,bytes}.Replace usages hardcore -1.

That's hardcore.

@gopherbot
Copy link

Change https://golang.org/cl/137855 mentions this issue: bytes, strings: add ReplaceAll

@gopherbot
Copy link

Change https://golang.org/cl/137856 mentions this issue: all: use strings.ReplaceAll and bytes.ReplaceAll where applicable

gopherbot pushed a commit that referenced this issue Sep 26, 2018
I omitted vendor directories and anything necessary for bootstrapping.
(Tested by bootstrapping with Go 1.4)

Updates #27864

Change-Id: I7d9b68d0372d3a34dee22966cca323513ece7e8a
Reviewed-on: https://go-review.googlesource.com/137856
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@ferhatelmas
Copy link

@bradfitz don't you think following encourages bad naming patterns since new is a built-in ?

func Replace(s, old, new string, n int) string

reviewer: don't use built-in as an argument name ?
owner: std lib uses too
reviewer: ...?

@dominikh
Copy link
Member

Shadowing built-ins is an explicit feature of the language; nobody will get confused by a variable named new in the context of a replace function.
Also, this isn't the right place for questions regarding style. You can ask those on one of the various forums or mailing lists.

@ferhatelmas
Copy link

@dominikh thanks for your insight.

this isn't the right place .... You can ask those on one of the various forums or mailing lists.

Sorry for noise. Will be more careful about it next time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests