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: strings: add TrimPrefix2 and TrimSuffix2, variants which report whether they trimmed #42537

Open
cespare opened this issue Nov 12, 2020 · 7 comments

Comments

@cespare
Copy link
Contributor

@cespare cespare commented Nov 12, 2020

Inspired by #40135, I have another proposal for an addition to the strings package (and by extension, I suppose, the bytes package).

The strings functions I most frequently wish for are variants of TrimPrefix and TrimSuffix which report whether they did any trimming.

// TrimPrefix2 returns s without the provided leading prefix string
// and reports whether it found the prefix.
// If s doesn't start with prefix, TrimPrefix2 returns s, false.
// If prefix is the empty string, TrimPrefix2 returns s, true.
func TrimPrefix2(s, prefix string) (trimmed string, ok bool)

(About the name: "2" may refer to the fact that it's the second TrimPrefix function or to the 2 return parameters. It's not a great name and perhaps someone can come up with a better one.)

Lacking these functions, Go authors resort to fragile code, often writing a prefix/suffix literal twice or hard-coding its length:

if strings.HasPrefix(s, "myprefix") {
	s = strings.TrimPrefix(s, "myprefix")
	...
}
if strings.HasPrefix(s, "myprefix") {
	s = s[len("myprefix"):]
	...
}
if strings.HasPrefix(s, "myprefix") {
	s = s[8:]
	...
}
if t := strings.TrimPrefix(s, "myprefix"); s != t {
	// had prefix
	s = t
	...
}

Of course, a function like TrimPrefix2 is easy to write and can exist outside of the standard library. At my company we have these functions in an internal string helper package and they see regular use. But certainly I wouldn't pull in a dependency for such a tiny helper function and so when I'm working on my own projects I generally just use the above workarounds.


Here are some examples from the Go source tree along with how they could be altered to use TrimPrefix2/TrimSuffix2:

  • src/cmd/go/internal/modload/load.go:

    if strings.HasPrefix(suffix, "/vendor/") {
    	return strings.TrimPrefix(suffix, "/vendor/")
    }
    

    becomes

    if v, ok := strings.TrimPrefix2(suffix, "/vendor/"); ok {
    	return v
    }
    
  • src/cmd/go/proxy_test.go:

    if !strings.HasSuffix(name, ".txt") {
    	continue
    }
    name = strings.TrimSuffix(name, ".txt")
    

    becomes

    name, ok := strings.TrimSuffix2(name, ".txt")
    if !ok {
    	continue
    }
    
  • src/testing/benchmark.go:

    if strings.HasSuffix(s, "x") {
    	n, err := strconv.ParseInt(s[:len(s)-1], 10, 0)
    	...
    }
    

    becomes

    if s, ok := strings.TrimSuffix2(s, "x"); ok {
    	n, err := strconv.ParseInt(s, 10, 0)
    	...
    }
    
  • src/testing/fstest/mapfs.go:

    if strings.HasPrefix(fname, prefix) {
    	felem := fname[len(prefix):]
    	...
    }
    

    becomes

    if felem, ok := strings.TrimPrefix2(fname, prefix); ok {
    	...
    }
    
  • src/mime/mediatype.go:

    if !strings.HasPrefix(rest, ";") {
    	return "", "", v
    }
    
    rest = rest[1:] // consume semicolon
    

    becomes

    rest, ok := strings.TrimPrefix2(rest, ";")
    if !ok {
    	return "", "", v
    }
    
  • test/run.go:

    if strings.HasPrefix(line, "//") {
    	line = line[2:]
    } else {
    	continue
    }
    

    becomes

    line, ok := strings.TrimPrefix2(line, "//")
    if !ok {
    	continue
    }
    
  • test/run.go:

    if strings.HasPrefix(m, "LINE+") {
    	delta, _ := strconv.Atoi(m[5:])
    	n += delta
    	...
    }
    

    becomes

    if d, ok := strings.TrimPrefix2(m, "LINE+"); ok {
    	delta, _ := strconv.Atoi(d)
    	n += delta
    	...
    }
    
  • src/runtime/testdata/testprog/traceback_ancestors.go:

    if strings.HasPrefix(tb[pos:], "goroutine ") {
    	id := tb[pos+len("goroutine "):]
    	...
    }
    

    becomes

    if id, ok := strings.TrimPrefix2(tb[pos:], "goroutine "); ok {
    	...
    }
    
@gopherbot gopherbot added this to the Proposal milestone Nov 12, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Nov 12, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 12, 2020

TrimmedPrefix and TrimmedSuffix ?

@mvdan
Copy link
Member

@mvdan mvdan commented Nov 12, 2020

I find if t := strings.TrimPrefix(s, "myprefix"); s != t { reasonable, but that might just be because I've gotten used to the pattern. One argument in favor of it not being descriptive/intuitive enough is that it's not completely winning over alternatives like HasPrefix+TrimPrefix.

@jimmyfrasche
Copy link
Member

@jimmyfrasche jimmyfrasche commented Nov 12, 2020

CutPrefix and CutSuffix so they group with the proposed Cut in docs?

@rsc rsc moved this from Incoming to Active in Proposals Nov 18, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Nov 18, 2020

It's unclear whether we need these, but at the very least we'd need better names.
Adding to the minutes in hopes that someone will come up with better names.

@narqo
Copy link
Contributor

@narqo narqo commented Nov 20, 2020

(With all respect) It feels the proposal lacks the use cases where the new functions required. Knowing that the current implementation of both strings.TrimPrefix and strings.TrimSuffix is the following

func TrimPrefix(s, prefix string) string {
	if HasPrefix(s, prefix) {
		return s[len(prefix):]
	}
	return s

}

can't one always use TrimPrefix for all the samples from the issue's description, dropping Has-checks? I.e.

- if strings.HasPrefix(suffix, "/vendor/") {
- 	return strings.TrimPrefix(suffix, "/vendor/")
- }
+ return strings.TrimPrefix(suffix, "/vendor/")
@cespare
Copy link
Contributor Author

@cespare cespare commented Nov 20, 2020

@narqo no. The places where you use TrimSuffix2 do something different if the test string contains the prefix/suffix.

For example, you suggested

- if strings.HasPrefix(suffix, "/vendor/") {
- 	return strings.TrimPrefix(suffix, "/vendor/")
- }
+ return strings.TrimPrefix(suffix, "/vendor/")

but those are not the same thing, because your version returns if suffix does not start with "/vendor/" and the original does not.

I listed the filenames that all my examples come from. You can go check out that example in context and you'll see it looks like this:

if strings.HasPrefix(suffix, "/vendor/") {
	return strings.TrimPrefix(suffix, "/vendor/")
}
return targetPrefix + suffix
@rsc
Copy link
Contributor

@rsc rsc commented Dec 2, 2020

Putting on hold with Cut.

@rsc rsc moved this from Active to Hold in Proposals Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants