Maintenance and dependency updates

nknorg / nkn-sdk-go

Go implementation of NKN client and wallet

Apache License 2.0

58 stars 28 forks source link

Maintenance and dependency updates #138

Closed nbdy closed 2 weeks ago

nbdy commented 3 weeks ago

As per commit messages:

Renamed variables so they don't collide
Removed redundant aliases
Use errors.Is instead of equality operators
Add translation and fix grammar issues
Update dependencies
Replace mergo with copier (If ncp-go pull 11 is accepted we can phase out mergo completely)

yilunzhang commented 3 weeks ago

Thanks for the contribution! I have two comments:

Just like https://github.com/nknorg/ncp-go/pull/11, switching from mergo to copier would need more testing (and solid reasons). For example, mergo.WithOverride will make merge override non-empty dst attributes with non-empty src attributes values., while it seems you would need extra tags to achieve the same behavior in copier. I can't tell if there are other subtle difference in short time, so I would say we don't use copier in this PR.
Changing the go version in go.mod will break the pipeline in some of our systems using go < 1.22, so please don't change it in this PR.

Other changes look good to me

nbdy commented 3 weeks ago

I've described in ncp-go#11 why we could benefit from moving away from mergo (even if it's just slightly). I am not a fan of using vanity URLs to specify a module. Personal domains usually "expire" quicker than corporate ones. It would not be the first time that dario.cat has expired. I don't think this will happen to github.com. Also at github.com we can trust the served content a bit more.

Does copier.CopyWithOption(&dst, &src, copier.Option{IgnoreEmpty: true}) not exhibit the same functionality as mergo.Merge(&dst, src, mergo.WithOverride)? The tests pass. I am willing to add more tests for this if needed.

yilunzhang commented 3 weeks ago

I took a closer look and it seems copier.CopyWithOption(&dst, &src, copier.Option{IgnoreEmpty: true}) is the same as mergo.Merge(&dst, src, mergo.WithOverride) so it should be compatible.

What about the go version? If you need higher go version, you can specify that in your app that is importing this library. Setting the version too high here is essentially a breaking change for some CI system...

nbdy commented 3 weeks ago

We would not be able to upgrade golang.org/x/crypto to v0.27.0 unless we bump the go version. The newest version of crypto, which supports go 1.18 seems to be v0.24.0. I've set the go version to 1.20 (February 2023) for the project now. It was automatically set to 1.22 (February 2024) by GoLand, since that was my default toolchain.