refactor: add more linters#64
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enables additional golangci-lint linters to improve code quality and consistency. The main changes include updating nolint directives to satisfy new linters, refactoring code to fix violations, and improving error handling patterns.
Key Changes:
- Enabled multiple new linters (errorlint, exhaustive, fatcontext, forbidigo, forcetypeassert, funlen, etc.)
- Refactored duplicate icon processing code into reusable functions
- Improved error handling with
errors.Asinstead of type assertions - Enhanced test logging by using
t.Helper()andt.Logmethods
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.golangci.yml |
Enabled 10 new linters and added exclusion rule for cmd/migrate/main.go |
internal/websocket/common/hub_pool_test.go |
Added forcetypeassert to nolint directive for type assertion |
internal/websocket/common/hub.go |
Added forcetypeassert to nolint directive for type assertion |
internal/common/http/validate.go |
Replaced type assertion with errors.As for better error handling |
internal/api/support/ports/http.go |
Extracted error handling logic into separate handleError method |
internal/api/mobile/service/service.go |
Added funlen exemption for initialization function |
internal/api/icons/service/service.go |
Added funlen exemption for initialization function |
internal/api/icons/app/command/icons_requests.go |
Refactored duplicate icon processing into saveIcons and saveIcon helper functions |
internal/api/browser_extension/service/service.go |
Added funlen exemption for initialization function |
internal/api/browser_extension/app/command/request_2fa_token.go |
Extracted paired device lookup and notification sending into separate methods |
e2e-tests/http.go |
Improved test logging by adding t.Helper() and using t.Log instead of fmt.Printf |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| func saveIcon(iconURL, serviceName, iconType string, iconsStorage storage.FileSystemStorage, iconsRepository domain.IconsRepository) (uuid.UUID, error) { | ||
| storagePath := filepath.Join(iconsStoragePath, iconURL) |
There was a problem hiding this comment.
The function receives iconURL as a parameter (e.g., a full URL like 'https://example.com/icon.png'), but then passes it directly to filepath.Join with iconsStoragePath. This is incorrect because the original code used filepath.Base(iconRequest.LightIconUrl) to extract just the filename. The iconURL parameter should be processed with filepath.Base() to extract only the filename before joining it with the storage path.
| storagePath := filepath.Join(iconsStoragePath, iconURL) | |
| iconFileName := filepath.Base(iconURL) | |
| storagePath := filepath.Join(iconsStoragePath, iconFileName) |
| return iconsIds, nil | ||
| } | ||
|
|
||
| func saveIcon(iconURL, serviceName, iconType string, iconsStorage storage.FileSystemStorage, iconsRepository domain.IconsRepository) (uuid.UUID, error) { |
There was a problem hiding this comment.
The parameter name iconType is misleading. Based on the context and usage (domain.Light, domain.Dark), this represents the icon variant or theme, not a file type. Consider renaming to iconVariant or iconTheme for clarity.
| if err == nil { | ||
| return domain.WebServiceAlreadyExistsError{Name: iconRequest.ServiceName} | ||
| } else { | ||
| var notFound adapters.WebServiceCouldNotBeFoundError | ||
| if !errors.As(err, ¬Found) { | ||
| return fmt.Errorf("failed to find web service by name: %w", err) | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
This function violates single responsibility by both checking existence and handling the 'not found' error case. The else block at line 405 containing the error type check adds unnecessary complexity. Consider simplifying by inverting the logic: check for the specific 'not found' error type first, then return the AlreadyExistsError if no error occurred.
| if err == nil { | |
| return domain.WebServiceAlreadyExistsError{Name: iconRequest.ServiceName} | |
| } else { | |
| var notFound adapters.WebServiceCouldNotBeFoundError | |
| if !errors.As(err, ¬Found) { | |
| return fmt.Errorf("failed to find web service by name: %w", err) | |
| } | |
| } | |
| return nil | |
| if err != nil { | |
| var notFound adapters.WebServiceCouldNotBeFoundError | |
| if errors.As(err, ¬Found) { | |
| return nil | |
| } | |
| return fmt.Errorf("failed to find web service by name: %w", err) | |
| } | |
| return domain.WebServiceAlreadyExistsError{Name: iconRequest.ServiceName} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return nil, err | ||
| } | ||
|
|
||
| tokenRequestId, _ := uuid.Parse(cmd.Id) |
There was a problem hiding this comment.
This is already validated right?
There was a problem hiding this comment.
Seems like so, but I still added error check here. Previously this was just copy-pasted. Thanks for catching that!
| pairedDevices := h.PairedDevicesRepository.FindAll(browserExtension.Id) | ||
| return pairedDevices, nil |
There was a problem hiding this comment.
nit
| pairedDevices := h.PairedDevicesRepository.FindAll(browserExtension.Id) | |
| return pairedDevices, nil | |
| return h.PairedDevicesRepository.FindAll(browserExtension.Id), nil |
No description provided.