all: Adopt recommended linters in CI and address the problems pointed by them#142
Merged
marcelosalloum merged 27 commits intomainfrom Apr 18, 2025
Merged
Conversation
…backend" -l -w .`
…se transaction, and there would be no value in doing so. Kinda bogus code.
…ls functions are meant to be used from within tests by requiring a test object and handling the internal error inside of it.
…ts. This exposed some issues that will be handled in the next commit.
…surfaced thanks to better error handling.
There was a problem hiding this comment.
Pull Request Overview
This PR updates the CI configuration to run linters correctly and fixes several issues flagged by the linters such as error wrapping, incorrect error messages, variable shadowing, and test assertions. Key changes include:
- Improving error handling (using fmt.Errorf with %w) and consistent error messages.
- Refactoring test files to use testify/require and reorganizing imports.
- Updating CI workflows and linter configurations to enforce code standards.
Reviewed Changes
Copilot reviewed 101 out of 101 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/data/payments_test.go | Removed duplicate import lines and reordered imports for clarity |
| internal/data/payments.go | Improved error checking by using errors.Is for sql.ErrNoRows |
| internal/data/fixtures.go | Reordered import statements with minor cleanup |
| internal/data/accounts_test.go | Cleaned up blank lines in test setup |
| internal/apptracker/sentry/sentry_tracker*.go | Updated error wrapping and test setups for Sentry tracker functionality |
| internal/apptracker/sentry/mocks.go | Added helper for mock setup with t.Cleanup for restoration of function pointers |
| cmd/utils/* | Updated error messages and error wrapping in signature client handling |
| cmd/utils/password_prompter.go | Added error wrapping for IO related calls |
| cmd/migrate.go, cmd/ingest.go, cmd/distribution_account.go | Minor error wrapping and formatting improvements |
| .golangci.yml & .github/workflows/go.yaml | Updated linter configurations and CI workflows with improved caching |
Comments suppressed due to low confidence (1)
internal/apptracker/sentry/sentry_tracker.go:37
- Ensure that deferring FlushFunc (and similarly, RecoverFunc on the next line) is appropriate even when the Sentry initialization fails. Consider calling these functions only upon successful initialization to prevent unintended side effects.
defer FlushFunc(time.Second * time.Duration(flushFreq))
marcelosalloum
commented
Apr 16, 2025
| func (t *transactionService) BuildAndSignTransactionWithChannelAccount(ctx context.Context, operations []txnbuild.Operation, timeoutInSecs int64) (*txnbuild.Transaction, error) { | ||
| var tx *txnbuild.Transaction | ||
| var channelAccountPublicKey string | ||
| err := db.RunInTransaction(ctx, t.DB, nil, func(dbTx db.Transaction) error { |
Collaborator
Author
There was a problem hiding this comment.
I removed the call to RunInTransaction here because it was not being used. The dbTx was not being applied anywhere and considering there's only one database query, at the very last step here, then the database transaction serves no purpose here.
marcelosalloum
commented
Apr 16, 2025
| if errors.Is(err, store.ErrPublicKeyAlreadyExists) { | ||
| return err | ||
| } | ||
| return fmt.Errorf("storing distribution account encrypted private key: %w", err) |
Collaborator
Author
There was a problem hiding this comment.
I removed this assertion because the function returns the same error anyway, and it can be parsed downstream using errors.Is
marcelosalloum
changed the title
marcelosalloum
changed the title
marcelosalloum
added
enhancement
marcelosalloum
force-pushed
the
marcelo/polish-project-config-and-CI
branch
from
8fa8c7f to
38f1464
Compare
…r!) by allowing retryCounts and retryIntervals to be (optionally) parametrized.
…cy, and start using the EXPECT() method, which has better syntax.
aristidesstaffieri
approved these changes
Apr 18, 2025
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
Adopt recommended linters in CI and address the problems pointed by them
Fixes include:
GET /tss/transactions/{hash}would return an invalid JSON when the resource was not foundfmt.Errorf("{message}: %w"), which made issues hard to debug.errors.Asanderrors.Is, so wrapped errors can also be captured....Error) and error variables (should start withErr...) to be go idiomatic.Additionally:
mocks.go, for consistency.goimportsWhy
It's good practice to use linters, specially the ones that capture errors and help making the code more debuggable and idiomatic.
Checklist
PR Structure
allif the changes are broad or impact many packages.Thoroughness
Release