This repository has been archived by the owner on Jun 28, 2023. It is now read-only.
Go 1.16 support and some general improvements #350
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
golint has been deprecated and archived. There's no 1:1 match for it, but staticcheck is a checker the golint README recommends and has tons of useful checks. This commit enables all checks, and then disables: * ST1000: Incorrect or missing package comment * ST1005: Incorrectly formatted error string, necessary due to the fact that all our error strings are capitalised, which they should not be * SA1019: Locally in goneb.go because we have to rework how we use the prometheus package there It also fixes a number of other errors surfaced by staticheck.
v2 was a set of erroneous tags, the latest is v1.14.7 which also fixes a vet warning related to building sqlite3 on gcc10+.
|
Though all the tests pass, one test is logging a panic: I don't really know why, it seems the handler that gets installed for |
|
@kegsay @neilalexander Could I bother you for a review on this? |
There was a problem hiding this comment.
Might be better to use golangci-lint, which includes staticcheck. There's a sample config in the Dendrite repository, if we're looking to settle on a standard.
I was thinking about that, but due to the amount of extra linters that it has and the state of go-neb that becomes a lot of extra things to fix. I'm happy to do that, but I'd prefer to do it as a follow-up PR. |
daenney
commented
May 19, 2021
|
I realized you also incorporate my changes: https://github.com/matrix-org/go-neb/pull/351/files |
|
@neilalexander @kegsay Could I get another review and/or sign-off please? I'd like to move this forward. |
kegsay
reviewed
Jul 20, 2021
kegsay
approved these changes
Jul 20, 2021
@kegsay Do you have any thoughts on this? I'm still kinda puzzled by the test. It feels wrong that it's logging a panic there. |
|
Let's leave it for now, but if you can file an issue for it that'd be great. |
This was referenced Jul 21, 2021
daenney
added a commit
that referenced
this pull request
Jul 21, 2021
Forgot to do this as part of #350, which flipped CI to GitHub Actions.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just for general house keeping purposes and getting access to new features in go
Golint has been deprecated and frozen
The v2 tags were erroneous, move us back to v1 tags
We've started to use GHA for more things on matrix-org, this moves neb in the same direction. The one thing we lose now is the Travis-CI webhook notifications, but given GHA will poke the author of the PR if their builds fail that feels like an acceptable trade-off. You can see what a build ends up looking like over here
We need this in order to be able to build with newer versions of Go