Skip to content
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

Merged
merged 10 commits into from
Dec 12, 2023
Merged

Bump Golang version #401

merged 10 commits into from
Dec 12, 2023

Conversation

matiasinsaurralde
Copy link
Contributor

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 than sort 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.

  • New packages/exported functions have docstrings.
  • New/changed functionality is thoroughly tested.
  • New/changed functionality has a function giving an example of its usage in the associated test file. See primers/primers_test.go for what this might look like.
  • Changes are documented in CHANGELOG.md in the [Unreleased] section.
  • All code is properly formatted and linted.
  • The PR template is filled out.

@matiasinsaurralde
Copy link
Contributor Author

My only doubt here comes around c5a01d0, in theory it's properly using the weightedrand method to set a random source however tests fail. Would appreciate any feedback on it.

@TimothyStiles
Copy link
Collaborator

@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.

Copy link
Collaborator

@carreter carreter left a 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
Copy link
Collaborator

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?

Copy link
Collaborator

@TimothyStiles TimothyStiles Dec 12, 2023

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.

Copy link
Collaborator

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?

Copy link
Collaborator

@TimothyStiles TimothyStiles left a 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!

@TimothyStiles TimothyStiles merged commit 7b3bb48 into bebop:main Dec 12, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants