search

pelletier / go-toml

Go library for the TOML file format
https://github.com/pelletier/go-toml
Other
1.72k stars 209 forks source link

Minimum Go version supported? #898

Closed JRaspass closed 1 year ago

JRaspass commented 1 year ago

So I have a draft PR (#899) that replaces all the uses of the deprecated io/ioutil package in this module with either io or os, this is valid since Go 1.16. I thought this would be okay since the go.mod file specifies go 1.16 in it. However I have since learnt that this isn't enforced until 1.21:

The go directive sets the minimum version of Go required to use this module. Before Go 1.21, the directive was advisory only; now it is a mandatory requirement: Go toolchains refuse to use modules declaring newer Go versions. -- https://go.dev/doc/modules/gomod-ref#go-notes

So I decided to edit our CI runner to find out what our actual minimum Go version is and it turns out it's 1.17, because of testify:

Error: ../../../go/pkg/mod/github.com/stretchr/testify@v1.8.4/assert/assertions.go:94:23: field.IsExported undefined (type reflect.StructField has no field or method IsExported) -- https://github.com/JRaspass/go-toml/actions/runs/6136791987/job/16651974610

So I know there's #872 to remove testify eventually but I also don't think picking a minimum version or ideally a minimum number of versions is a bad thing. There's nothing groundbreaking in each Go release but instead we get little nice to have features that eventually add up to a much nicer workflow, like the t.TempDir() from 1.15 in my PR.

pelletier commented 1 year ago

First of all, thank you very much for looking into it! Really appreciate the effort and starting the conversation.

go-toml is officially tested on the last two versions of Go (following their own policy), but I usually try to let the library run (not test) on older go versions if not too inconvenient. As far as I know, go-toml still runs on go 1.11. This version is old, ~but it is the version officially supported by Google AppEngine at the moment (1.12+ is still in beta)~ (I stand corrected, AppEngine supports up to 1.20 in GA now). I know people who rely on that environment, and a quick code search seems to indicate that some people are in the same boat.

I think just changing ioutil to os imports isn't worth breaking version compatibility here. A bigger reason to do so eventually may be better support for generics (though depending on how it's done it may be worth looking into a build tag for this).

JRaspass commented 1 year ago

Ah that makes sense, I'll close this and the PR then. Thanks.