proposal: path/filepath: add TrimExt

kybin commented 6 years ago

It would be good to add filepath.TrimExt function.

Which will do as it's name says. For example,

filepath.TrimExt("some/path/main.go") // returns "some/path/main"
filepath.TrimExt("main.go") // returns "main"

Reason

We often trims an extension from a path but there is no API for this, so we do like this now.

path[:len(path)-len(filepath.Ext(path))] 
// or
strings.TrimSuffix(path, filepath.Ext(path))

Which is cumbersome to type and hard to read.

The second one is easier to read, but user should import "strings" package for it.

Implemetation

It's implemation will be quite small.

func TrimExt(path string) string {
    return path[:len(path)-len(Ext(path))]
}

If this proposal accepted, I could work for this.

Note

The "path" package also has Ext. But it is not frequently used, so I don't think it should be also added. If I am wrong, please correct me.

agnivade commented 6 years ago

Given that you can easily get this functionality by using existing standard library functions, I don't see much value for this to exist in standard library. Mostly because it fails https://golang.org/doc/faq#x_in_std test.

Also, adding one more line makes it much more readable.

extLen := len(filepath.Ext(path))
path[:len(path)-extLen]

Although, IMO the one-liner is perfectly readable.

rsc commented 6 years ago

path[:len(path)-len(filepath.Ext(path))]

Can you show that this comes up a lot in code today? I can't think of any time I've ever used this expression. Usually I write something like strings.TrimSuffix(path, ".go")+".o", where there's exactly one suffix that can be trimmed but others must not be.

kybin commented 6 years ago

Thank you @agnivade for your thought.

I think we are used to it, but there is a room for making it better.

For the x_in_std test, I think it's small, complement addition to the "filepath" package. And it's maintenance cost seems not much.

kybin commented 6 years ago

Thanks @rsc . I should've verify that first.

I did quick check with go-corpus data.

I removed vendored results, if they are already included.

With your argument about strings.TrimSuffix(path, ".go")+".o", I excluded the similar cases.

It's not tremendous, still many. But I don't know you think they are enough.

-len(filepath.Ext(path)) pattern

$ rg "[-]len\(filepath.Ext"

github.com/spf13/hugo/tpl/template.go
285:    name = name[:len(name)-len(filepath.Ext(innerPath))] + ".html"

github.com/zyedidia/micro/cmd/micro/rtfiles.go
42: return fn[:len(fn)-len(filepath.Ext(fn))]

github.com/yosssi/ace/cmd/ace/main.go
42: base := baseFile[:len(baseFile)-len(filepath.Ext(baseFile))]
46:     inner = innerFile[:len(innerFile)-len(filepath.Ext(innerFile))]

$ rg "[-]len\(ext"

github.com/golang/protobuf/protoc-gen-go/generator/generator.go
320:        name = name[:len(name)-len(ext)]

github.com/pingcap/pd/_vendor/vendor/github.com/unrolled/render/render.go
182:                name := (rel[0 : len(rel)-len(ext)])
227:                name := (rel[0 : len(rel)-len(ext)])

github.com/ulikunitz/xz/cmd/gxz/file.go
137:        target = path[:len(path)-len(ext)]

github.com/CodisLabs/codis/vendor/github.com/martini-contrib/render/render.go
221:                name := (r[0 : len(r)-len(ext)])

github.com/spf13/hugo/hugolib/permalinks.go
159:    //var name = p.Source.Path()[0 : len(p.Source.Path())-len(extension)]

github.com/spf13/hugo/target/file.go
65: return f[:len(f)-len(ext)]

github.com/gogo/protobuf/protoc-gen-gogo/generator/generator.go
356:        name = name[:len(name)-len(ext)]

github.com/ncw/rclone/b2/api/types.go
76: base := remote[:len(remote)-len(ext)]
90: base := remote[:len(remote)-len(ext)]

github.com/ncw/rclone/fs/operations.go
939:    base := remote[:len(remote)-len(ext)]

github.com/couchbase/go-couchbase/tools/loadfile/loadfile.go
52: return file[0 : len(file)-len(ext)]

github.com/git-lfs/git-lfs/lfsapi/ssh.go
83:         basessh = basessh[:len(basessh)-len(ext)]

github.com/openshift/origin/test/integration/template_test.go
104:            name = name[:len(name)-len(ext)]

github.com/openshift/origin/vendor/gopkg.in/natefinch/lumberjack.v2/lumberjack.go
233:    prefix := filename[:len(filename)-len(ext)]
371:    filename = filename[:len(filename)-len(ext)]
393:    prefix = filename[:len(filename)-len(ext)] + "-"

github.com/openshift/origin/examples/examples_test.go
50:         name = name[:len(name)-len(ext)]

github.com/kataras/go-template/html/html.go
215:            root := name[:len(name)-len(ext)]

github.com/kardianos/service/example/runner/runner.go
115:    name := execname[:len(execname)-len(ext)]

golang.org/x/tools/go/gcimporter15/gcimporter_test.go
118:                    name := f.Name()[0 : len(f.Name())-len(ext)] // remove extension

golang.org/x/tools/blog/blog.go
192:        p = p[len(root) : len(p)-len(ext)] // trim root and extension

$ rg "[-]len\(\"[.]"

github.com/klauspost/compress/flate/huffman_bit_writer_test.go
34:         out = in[:len(in)-len(".in")] + ".golden"

github.com/xenolf/lego/providers/dns/gandi/gandi.go
92: name := fqdn[:len(fqdn)-len("."+authZone)]

github.com/xenolf/lego/providers/dns/exoscale/exoscale.go
129:    name = name[:len(name)-len("."+zone)]

github.com/gopherjs/gopherjs/tests/run.go
856:    filename = filename[:len(filename)-len(".go")]

github.com/chrislusf/seaweedfs/weed/storage/disk_location.go
29:     base := name[:len(name)-len(".dat")]

github.com/iris-contrib/lego/providers/dns/gandi/gandi.go
92: name := fqdn[:len(fqdn)-len("."+authZone)]

github.com/newrelic/go-agent/internal/sysinfo/docker.go
83:         id = string(cols[2][len("/system.slice/docker-") : len(cols[2])-len(".scope")])

golang.org/x/tools/cmd/stringer/endtoend_test.go
61:     typeName := fmt.Sprintf("%c%s", name[0]+'A'-'a', name[1:len(name)-len(".go")])

golang.org/x/tools/cmd/godoc/codewalk.go
230:            v = append(v, &elem{name[0 : len(name)-len(".xml")], cw.Title})

golang.org/x/image/font/sfnt/sfnt_test.go
269:    } else if want := filename[:len(filename)-len(".ttf")]; name != want {

strings.TrimSuffix(... filepath.Ext(path)) pattern

$ rg "strings.TrimSuffix\(.*[, ]+filepath[.]Ext.*\)"

github.com/spf13/hugo/tpl/template.go
317:        templateName := strings.TrimSuffix(name, filepath.Ext(name)) + ".html"

github.com/onsi/ginkgo/ginkgo/testsuite/test_suite.go
38: packageName := strings.TrimSuffix(filepath.Base(path), filepath.Ext(path))

github.com/future-architect/vuls/commands/history.go
86:         fileBase := strings.TrimSuffix(f.Name(), filepath.Ext(f.Name()))

github.com/docker/docker/pkg/plugins/discovery.go
37:         name := strings.TrimSuffix(fi.Name(), filepath.Ext(fi.Name()))
50:         name := strings.TrimSuffix(fi.Name(), filepath.Ext(fi.Name()))

github.com/docker/docker/vendor/github.com/docker/notary/trustmanager/keystore.go
263:    name := strings.TrimSpace(strings.TrimSuffix(filepath.Base(keyID), filepath.Ext(keyID)))

github.com/flynn/flynn/test/cluster/instance.go
174:    name := strings.TrimSuffix(filepath.Base(image), filepath.Ext(image))

github.com/flynn/flynn/vendor/github.com/jvatic/asset-matrix-go/scss-asset.go
153:                    _p = strings.TrimSuffix(_p, filepath.Ext(_p)) + "-" + a.r.cacheBreaker + filepath.Ext(_p)

$ rg "strings.TrimSuffix\(.*[, ]+ext.*\)"

github.com/influxdata/influxdb/tsdb/engine/tsm1/tombstone.go
324:        filename = strings.TrimSuffix(filename, ext)

github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/genswagger/generator.go
49:     base := strings.TrimSuffix(name, ext)

github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway/descriptor/registry.go
296:        return strings.TrimSuffix(base, ext)

github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway/gengateway/generator.go
79:     base := strings.TrimSuffix(name, ext)

github.com/gogits/gogs/routers/repo/repo.go
287:    refName = strings.TrimSuffix(uri, ext)

github.com/openshift/origin/vendor/k8s.io/kubernetes/examples/examples_test.go
162:            name := strings.TrimSuffix(file, ext)

github.com/flynn/flynn/vendor/github.com/jvatic/asset-matrix-go/matrix.go
357:    outputPath = strings.TrimSuffix(outputPath, ext) + "-" + m.cacheBreaker + ext

rsc commented 6 years ago

Thanks for gathering these examples. I looked through a bunch of them. The ones that already have ext when they want to slice it off seem to typically be inserting a string in the middle of the path, between the part before the extension and the extension. That is, they add the extension back. So they need to know what the extension is, so TrimExt by itself does not simplify the surrounding code.

The ones that know the specific extension they want to trim have already checked it and are similarly unhelped by a general TrimExt.

The ones that remain, that blindly strip any extension and return what is left, seem to me incorrect. It's hard for me to see in what context you'd want:

x.tar.gz -> x.tar
x -> x
x.tgz -> x
x.bash -> x
x-1.2.3 -> x-1.2

If you do want that, it's trivial to write, as we established. But I don't see that it's terribly useful for the library to provide it.

Let's leave this one out in the absence of compelling evidence that it has general utility.

kybin commented 6 years ago

@rsc Thanks for the analysing of TrimExt. And I agree.

Actually, what I imagined was filepath.TrimExt(f) + something + filepath.Ext(f). But it seems little bit inefficient.

Actually I am leaning towards to SplitExt. But I don't want to make a proposal in a hurry again.

golang / go