-
-
Notifications
You must be signed in to change notification settings - Fork 73
Bump Golang version #401
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
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. |
carreter
left a comment
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.
| 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?
TimothyStiles
left a comment
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
slicespackage which is recommended and more performant thansortmethods 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.gofor what this might look like.CHANGELOG.mdin the[Unreleased]section.