chore(lint): fix more lint errors, add lint to the pipeline - #63
Conversation
There was a problem hiding this comment.
Pull request overview
This PR focuses on fixing linting errors and adding linting to the CI pipeline. The changes primarily address Go code formatting issues, error handling improvements, and type naming conventions.
Key changes:
- Fixed import ordering and grouping across all Go files to follow Go conventions
- Improved error handling by checking returned errors from
json.Marshal()calls - Renamed error types to follow the
Errorsuffix convention for better Go idiom compliance - Enabled linting in the GitHub Actions workflow
Reviewed changes
Copilot reviewed 94 out of 94 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
Multiple *.go files |
Standardized import ordering (stdlib, external, internal packages with blank lines) |
internal/common/push/client.go |
Added error handling for json.Marshal() calls |
internal/common/logging/logger.go |
Added error handling for json.Marshal() in LogCommand |
internal/api/icons/app/queries/web_service_dump.go |
Added error handling for json.Marshal() |
internal/api/health/ports/http.go |
Added error handling for json.Marshal() |
| Error type files | Renamed error types from *CouldNotBeFound to *CouldNotBeFoundError |
internal/common/http/error.go |
Renamed ErrorResponse to ResponseError |
internal/api/mobile/app/queries/mobile_device.go |
Renamed error variable from MobileDeviceNotFound to ErrMobileDeviceNotFound |
internal/common/db/error.go |
Added //nolint:errname comment for linter exception |
.golangci.yml |
Commented out most linters, keeping only essential ones enabled |
.github/workflows/go.yml |
Added linting step to CI workflow |
| Multiple test files | Added error handling for json.Marshal() and fixed function signatures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| client, err := app.Messaging(context.Background()) | ||
| if err != nil { | ||
| logging.WithField("error", err.Error()).Fatal("Error initializing FCM Messaging client") |
There was a problem hiding this comment.
Do we really want to kill app here?
There was a problem hiding this comment.
At this point we already know we have FCM credentials. We have firebase application. If creating firebase application had failed, this would have been a fatal (a few lines above).
So I think if we fail to create messaging client for firebase app, this should kill app as well. This is consistent with the existing behaviour.
No description provided.