Closed GreyXor closed 6 months ago
Thatnks @GreyXor, but if the parses have different behaviour and the existing tests fail, that'll break backwards compatibility. If that's the case, then the parsers/toml
package should be versioned parsers/toml/v2
. Can you confirm whether it is indeed backwards incompatible?
Thatnks @GreyXor, but if the parses have different behaviour and the existing tests fail, that'll break backwards compatibility. If that's the case, then the
parsers/toml
package should be versionedparsers/toml/v2
. Can you confirm whether it is indeed backwards incompatible?
Thanks, I can confirm that this breaks backward compatibility. Should I move it into a subdirectory called v2
?
Yep, in that case, it should be /v2
. In your PR, you should update parsers/toml/go.mod
to have the v2 suffix (module github.com/knadh/koanf/parsers/toml/v2
).
https://github.com/knadh/koanf/blob/6c6c498fb49bb447a3826326903e5a0035a23ac2/parsers/toml/go.mod#L1
Yep, in that case, it should be
/v2
. In your PR, you should updateparsers/toml/go.mod
to have the v2 suffix (module github.com/knadh/koanf/parsers/toml/v2
).https://github.com/knadh/koanf/blob/6c6c498fb49bb447a3826326903e5a0035a23ac2/parsers/toml/go.mod#L1
Here it is, toml/v2 in a subdirectory
There should be no v2
subdirectory. Go versioning works based on the paths defined in go.mod and not directory structures. Please refer to this: https://github.com/knadh/koanf/tree/master/providers/consul
You should make the necessary changes to the toml
provider and simply upgrade the version path in its go.mod to have the /v2
suffix. Once it's merged, I'll push a tag that makes it go gettable.
I understood that: We want to keep both versions of the TOML parser, the current one for compatibility needs, and a new "v2", mine. To do this, I:
If a v2 subfolder doesn't suit you, where should I put it?
The current version already exists in the repo with a specific tag. You update the files to the new version and add /v2
and I will push a v2 tag for your commit. Thus, both versions will co exist in the repository's git history.
In the directory structure, there should also be the latest "version" (state) of the file. There shouldn't be subdirectories with different versions.
Thanks, I've deleted the v2 subfolder and parser/toml is now the v2. @knadh, it's all good for you?
Thanks @GreyXor. Will review and merge by this weekend.
Also, let's update the following:
examples/read-commandline/main.go
which uses the v1 package; to the /v2 version.tests
packageAlso, let's update the following:
* `examples/read-commandline/main.go` which uses the v1 package; to the /v2 version. * Add /v2 to README * Add a test to `tests` package
Thank you, i've done the two first bullets points from your list. Would you like a complete new test in a new file ? Or just a test directly in koanf_test.go
?
Thank you, i've done the two first bullets points from your list. Would you like a complete new test in a new file ? Or just a test directly in
koanf_test.go
?
https://github.com/knadh/koanf/blob/master/tests/koanf_test.go#L282-L288
Maybe we can just add a new case with tomlv2 as the parser?
import(
...
tomlv2 "github.com/knadh/koanf/parsers/toml/v2"
...
)
I've added a fix for the go.work and tests not running in the workflow. Can you rebase your PR on master?
Once this is done, we can go ahead and merge.
I've added a fix for the go.work and tests not running in the workflow. Can you rebase your PR on master?
Once this is done, we can go ahead and merge.
Thanks, I rebased on master
This is merged in #286. Thanks @GreyXor. Had to create a new PR out of your commits as I'd one minor change to make which couldn't be pushed to your PR (as it's from your master
and not a feature branch).
This is merged in #286. Thanks @GreyXor. Had to create a new PR out of your commits as I'd one minor change to make which couldn't be pushed to your PR (as it's from your
master
and not a feature branch).
Thanks :+1:
I understand, next time i'll push in a feature/foo
branch.
I've revised the TOML parser to make it compatible with the v2 of go-toml.
It appears that this updated version parses differently; specifically, it no longer includes spaces in subgroups and only returns nil if the map is empty(look at the tests to see the difference).
Please tell me if there are any additions or modifications required.