feat: added error codes #73
Conversation
Pull Request Test Coverage Report for Build 11289136607Details
💛 - Coveralls |
| @@ -11,7 +11,7 @@ jobs: | |||
| runs-on: ubuntu-latest | |||
| strategy: | |||
| matrix: | |||
| go-version: ["1.20"] | |||
| go-version: ["1.21"] | |||
There was a problem hiding this comment.
Is there a reason you aren't keeping 1.20 in this list and dropping it entirely?
I don't develop in Go, so not sure what the community stance is on backwards compatibility in libraries.
| @@ -1,3 +1,3 @@ | |||
| module github.com/uber/h3-go/v4 | |||
|
|
|||
| go 1.20 | |||
| go 1.21 | |||
There was a problem hiding this comment.
Again, why change the minimum version here? Are you using a feature that only exists in 1.21?
There was a problem hiding this comment.
Not really, usually most of the libraries support 3 latest versions of Go. Makes it easier for maintainers to maintain the package.
So it depends on h3-go’s backward comptability policies.
I can downgrade to Go 1.20 as well.
There was a problem hiding this comment.
Got it. As I said, I don't develop in Go, so I will leave that decision to @zachcoleman and/or @jogly
There was a problem hiding this comment.
I only care we support the versions of Go that Go cares to support (https://go.dev/doc/devel/release), and their policy is 2 versions. Go is at 1.23 now so I support dropping 1.20 😁
|
Overall, introducing breaking changes (even rightfully so), I personally want to think on for a bit. The repo versions with major and minor for upstream h3 (ref). So maybe this is fine, but definitely feels like this cutover will break a lot if users seemingly upgrade the patch version due to these semantics. @jogly Do you have an opinion on this? |
|
I've hemmed and hawed on this for a while myself. I agree w your hesitation, and am unwilling to release breaking changes as a patch version for the reasons you mention. My latest thought is to publish this in a v4.2 prerelease branch/tag and wait until core is at 4.2 to make it a full release. Another thought would be to consider an alternate strategy where we keep the package functions' signature as-is, and introduce an h3 struct with the signatures from this PR. Then the package functions could call an unexported default structs' functions and swallow the error for backwards compatibility and ergonomics. |
Why not a major release like v5? |
That has been the standard to date in the repo: Lines 6 to 8 in d58e7c6 |
|
I'm inclined to want to be patient and make this cutover on an h3-core 4.2 release. I reached out to h3-core team on Slack and they're inclined to release soon. I think a little patience here can go a long way and save some complicated work. Ref: https://h3-core.slack.com/archives/CPU7D1TRD/p1728850733866769 |
|
Thank you for your patience @mojixcoder... there's still no movement on the core library right now. I just wanted to poke this and let you know I'm checking frequently and we will act quickly after a core library releases and your contribution is very much appreciated ❤️. |
|
No worries, you can merge it whenever the core library is ready and thanks for your kind words @zachcoleman |
Breaking changes are introduced to the package's API.
Closes #56