-
-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bump Golang version #401
Bump Golang version #401
Conversation
74b5fe7
to
c5a01d0
Compare
My only doubt here comes around c5a01d0, in theory it's properly using the |
@carreter if we're bumping the version then we should probably also bump the go versions that CI/CD runners use. Doing it now before merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for single comment. murmur3
probably should be in the indirect dependencies section.
@@ -9,14 +9,14 @@ require ( | |||
github.com/mroth/weightedrand v0.4.1 | |||
github.com/pmezard/go-difflib v1.0.0 | |||
github.com/sergi/go-diff v1.2.0 | |||
github.com/spaolacci/murmur3 v1.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this moved to be a direct dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure @carreter. It's used only in the mash
package similar to how blake3 is only used in the seqhash
package. Both are direct dependencies here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah had to double check but murmur3 should have been a direct dependency to begin with @carreter. Good to merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Changes in this PR
Bumps Golang version to latest stable one: 1.21.4
Also ran
go mod tidy
.Why are you making these changes?
Just keeping it up to date and I also think there can be many benefits, for example I explored the new
slices
package which is recommended and more performant thansort
methods in a few scenarios, etc.Are any changes breaking? (IMPORTANT)
No - tests are fine too
Pre-merge checklist
All of these must be satisfied before this PR is considered
ready for merging. Mergeable PRs will be prioritized for review.
primers/primers_test.go
for what this might look like.CHANGELOG.md
in the[Unreleased]
section.