-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
refactor: fix various linter warnings #414
Conversation
- Extracted main logic into a separate `mainWithExitCode()` function - Added explicit return of exit codes for better error handling - Simplified main function to use the new exit code mechanism - Maintained existing profiling, configuration, and command execution logic
- Updated function signature to use comma-separated parameter list - Maintained existing functionality for converting weather icon codes - Minor formatting improvement for readability
- Updated function signature to use comma-separated parameter list - Maintained existing functionality for user ID validation - Minor formatting improvement for readability
- Updated Publish method signature to use comma-separated parameter list - Maintained existing functionality for publishing MQTT messages - Minor formatting improvement for readability
- Updated file open permissions from decimal to octal notation (0666 -> 0o666) - Maintained existing file handling functionality in DefaultFileHandler - Improved code readability by using Go's recommended octal literal format
- Added descriptive comments for ignored errors during logout process - Maintained existing logout functionality for all providers - Improved code readability by explaining why certain errors can be safely ignored
- Updated function signatures in template_functions.go to use pointer types - Maintained existing functionality for hourly data and count calculations - Improved type safety and consistency with pointer-based parameter passing
…teration - Updated ProcessNotes to use index-based iteration for efficiency - Modified sumHourlyCounts to accept pointer to hourly counts array - Removed commented-out logging and timing code - Simplified note processing logic while maintaining existing functionality
- Updated daily count calculation in createBirdVocalizationChart - Used index-based iteration for notes processing - Normalized date formatting to ensure consistent key generation - Maintained existing chart generation functionality
- Modified SaveSettings to use pointer-based comparisons for settings changes - Updated rangeFilterSettingsChanged and birdnetSettingsChanged functions to accept pointer parameters - Maintained existing functionality for detecting settings modifications - Improved type safety and consistency in settings handling
- Updated filename header generation to use quote formatting - Replaced string concatenation with more robust quoting method - Maintained existing file transfer functionality - Improved browser compatibility for filename encoding
- Updated addWeatherAndTimeOfDay to use index-based iteration - Simplified note processing by removing redundant variable assignments - Maintained existing functionality for adding weather and time of day information - Improved code efficiency by directly accessing notes slice elements
- Removed unnecessary dataStore parameter from clip cleanup functions - Updated bird image cache initialization to use index-based iteration - Maintained existing functionality for clip cleanup and image caching - Improved code efficiency by directly accessing slice elements
- Modified Publish method to accept pointer to Note - Updated RandomizeLocation method to use named return values - Maintained existing functionality for BirdWeather client interactions - Improved type safety and code clarity in birdweather package
- Replaced string.ToLower() with strings.EqualFold() for case-insensitive locale matching - Maintained existing locale normalization functionality - Improved code readability and comparison efficiency
- Updated directory creation mode to use octal literal (0o755) - Simplified retention period check with direct string comparison - Improved IsSoxAvailable function signature with named return values - Maintained existing functionality in configuration utility functions
…ection - Added nolint comment to suppress gocritic warning in model switch statement - Maintained existing logic for determining performance cores - Improved code linting compliance without changing functionality
- Updated file permission mode to use octal literal (0o644) - Maintained existing debug file writing functionality - Improved code consistency with modern Go octal literal syntax
- Modified Windows and macOS diagnostic collection functions to remove unnecessary tmpDir parameter - Updated file writing method in collectLegacyLogs to use fmt.Fprintf - Improved URL regex pattern matching with more precise syntax - Maintained existing diagnostic collection functionality across different platforms
- Simplified conditional logging in UsageBasedCleanup function - Removed redundant else block - Maintained existing debug logging functionality - Improved code readability and conciseness
…ore Save method - Restructured Save method to use an inner function for better error handling - Simplified database transaction retry logic - Improved error propagation and logging for database lock scenarios - Maintained existing transaction and save functionality
…n functions - Updated validateMySQLConfig and validateSQLiteConfig to remove unused settings parameter - Simplified database configuration validation method signatures - Maintained existing validation logic and error handling - Improved function clarity by removing redundant parameter
- Updated function signatures in DataStore methods to improve parameter ordering - Simplified parameter lists for GetClipsQualifyingForRemoval, SpeciesDetections, SearchNotes, and GetHourlyDetections - Modified getHourRange function to use named return values - Maintained existing functionality and logic across affected methods
…pture - Added nolint comments to suppress warnings for unchecked error returns in deferred function calls - Maintained existing error handling and audio capture functionality - Improved code linting compliance without changing core logic
- Added nolint comment to suppress gocritic warning for heavy parameter copy - Maintained existing deep copy functionality for Results struct - Improved code linting compliance without changing core logic
- Updated getPerformanceRating function to use named return values - Maintained existing performance rating logic - Improved function signature clarity and readability
WalkthroughThis pull request encompasses a comprehensive set of modifications across multiple files in the codebase, focusing primarily on refining function signatures, improving code clarity, and making syntax updates. The changes span various packages and include updates to method signatures, parameter handling, error management, and octal literal representations. While the modifications do not introduce significant new functionality, they enhance code readability, maintainability, and adhere to modern Go coding conventions. Changes
Sequence DiagramsequenceDiagram
participant Main as Main Program
participant Profile as Profiling
participant Config as Configuration
participant Execution as Command Execution
Main->>Profile: Create Profile
Profile-->>Main: Profile Status
Main->>Config: Load Configuration
Config-->>Main: Configuration Status
Main->>Execution: Execute Command
Execution-->>Main: Return Exit Code
Main->>Main: Exit with Code
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
internal/httpcontroller/handlers/settings.go (1)
Line range hint
191-216
: Consider refactoring complex code instead of suppressing linter warnings.While suppressing linter warnings is a temporary solution, the complexity warnings suggest that this code would benefit from refactoring. Consider breaking down the function into smaller, more focused functions.
Example approach:
- Extract type-specific handling into separate functions
- Create a strategy pattern for different field types
- Reduce nesting levels by early returns
Would you like me to propose a refactored version of this code?
🧹 Nitpick comments (5)
internal/httpcontroller/handlers/utils.go (1)
26-26
: Consider keeping the by-value parameter.While using a pointer avoids copying the array, it's not idiomatic Go to pass pointers to arrays that are only being read. The current array size (24 integers) is small enough that copying it won't cause performance issues, and the by-value approach provides better encapsulation.
-func sumHourlyCounts(hourlyCounts *[24]int) int { +func sumHourlyCounts(hourlyCounts [24]int) int { total := 0 - for _, count := range hourlyCounts { + for _, count := range hourlyCounts { total += count } return total }main.go (1)
Line range hint
35-86
: Consider using named constants for exit codes.While the error handling is good, using named constants for exit codes would improve maintainability and documentation.
+const ( + exitSuccess = 0 + exitError = 1 +) func mainWithExitCode() int { // ... existing code ... - return 1 + return exitError // ... more code ... - return 0 + return exitSuccess }internal/datastore/interfaces.go (1)
87-134
: Improved transaction error handling with retries.The new error handling implementation includes several improvements:
- Proper transaction rollback on panic
- Database lock detection and retries
- Consistent error wrapping
- Transaction ID for better logging
However, consider extracting the retry logic into a separate helper function to improve readability and reusability.
+func (ds *DataStore) executeWithRetry(txID string, maxRetries int, baseDelay time.Duration, fn func() error) error { + var lastErr error + for attempt := 0; attempt < maxRetries; attempt++ { + err := fn() + if err != nil { + if strings.Contains(strings.ToLower(err.Error()), "database is locked") { + delay := baseDelay * time.Duration(attempt+1) + log.Printf("[%s] Database locked, retrying in %v (attempt %d/%d)", txID, delay, attempt+1, maxRetries) + time.Sleep(delay) + lastErr = err + continue + } + return err + } + + if attempt > 0 { + log.Printf("[%s] Database transaction successful after %d attempts", txID, attempt+1) + } + return nil + } + return fmt.Errorf("[%s] failed after %d attempts: %w", txID, maxRetries, lastErr) +} func (ds *DataStore) Save(note *Note, results []Results) error { txID := fmt.Sprintf("tx-%s", uuid.New().String()[:8]) - maxRetries := 5 - baseDelay := 500 * time.Millisecond - var lastErr error - for attempt := 0; attempt < maxRetries; attempt++ { + return ds.executeWithRetry(txID, 5, 500*time.Millisecond, func() error { tx := ds.DB.Begin() if tx.Error != nil { - lastErr = fmt.Errorf("starting transaction: %w", tx.Error) - continue + return fmt.Errorf("starting transaction: %w", tx.Error) } - err := func() error { - defer func() { - if r := recover(); r != nil { - tx.Rollback() - } - }() + defer func() { + if r := recover(); r != nil { + tx.Rollback() + } + }() // Save the note and its associated results within the transaction if err := tx.Create(note).Error; err != nil { tx.Rollback() if strings.Contains(strings.ToLower(err.Error()), "database is locked") { return err } return fmt.Errorf("saving note: %w", err) } // ... rest of the function remains the same - } - return fmt.Errorf("[%s] failed after %d attempts: %w", txID, maxRetries, lastErr) + }) }internal/analysis/processor/actions.go (2)
Line range hint
215-220
: Consider simplifying the error handling pattern.The current if-else block can be flattened to improve readability:
- if err := a.BwClient.Publish(&a.Note, a.pcmData); err != nil { - log.Printf("error uploading to BirdWeather: %s\n", err) - return err - } else if a.Settings.Debug { - log.Printf("Uploaded %s to Birdweather\n", a.Note.ClipName) - } - return nil + err := a.BwClient.Publish(&a.Note, a.pcmData) + if err != nil { + log.Printf("error uploading to BirdWeather: %s\n", err) + return err + } + + if a.Settings.Debug { + log.Printf("Uploaded %s to Birdweather\n", a.Note.ClipName) + } + return nil
Line range hint
133-141
: Remove commented-out implementation ofSaveAudioAction.Execute
.This commented-out code is outdated and no longer relevant. Since the code is tracked in version control, it can be safely removed to improve code maintainability.
-/*func (a SaveAudioAction) Execute(data interface{}) error { - if err := myaudio.SavePCMDataToWAV(a.ClipName, a.pcmData); err != nil { - log.Printf("error saving audio clip to %s: %s\n", a.Settings.Realtime.Audio.Export.Type, err) - return err - } else if a.Settings.Debug { - log.Printf("Saved audio clip to %s\n", a.ClipName) - } - return nil // return an error if the action fails -}*/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
cmd/benchmark/benchmark.go
(1 hunks)internal/analysis/processor/actions.go
(1 hunks)internal/analysis/queue/queue.go
(1 hunks)internal/analysis/realtime.go
(5 hunks)internal/birdweather/birdweather.go
(3 hunks)internal/conf/locale.go
(1 hunks)internal/conf/utils.go
(3 hunks)internal/cpuspec/cpuspec.go
(1 hunks)internal/datastore/interfaces.go
(6 hunks)internal/datastore/mysql.go
(1 hunks)internal/datastore/sqlite.go
(1 hunks)internal/diagnostics/capture_debug.go
(1 hunks)internal/diagnostics/support_collect.go
(7 hunks)internal/diskmanager/policy_usage.go
(1 hunks)internal/httpcontroller/auth_routes.go
(1 hunks)internal/httpcontroller/handlers/dashboard.go
(1 hunks)internal/httpcontroller/handlers/detections.go
(1 hunks)internal/httpcontroller/handlers/media.go
(2 hunks)internal/httpcontroller/handlers/settings.go
(6 hunks)internal/httpcontroller/handlers/statistics.go
(1 hunks)internal/httpcontroller/handlers/utils.go
(1 hunks)internal/httpcontroller/template_functions.go
(3 hunks)internal/logger/filehandler.go
(2 hunks)internal/mqtt/client.go
(1 hunks)internal/myaudio/capture.go
(2 hunks)internal/security/oauth.go
(1 hunks)internal/weather/icon_codes.go
(1 hunks)main.go
(4 hunks)
✅ Files skipped from review due to trivial changes (9)
- internal/logger/filehandler.go
- internal/httpcontroller/auth_routes.go
- internal/cpuspec/cpuspec.go
- internal/analysis/queue/queue.go
- internal/diagnostics/capture_debug.go
- internal/security/oauth.go
- internal/myaudio/capture.go
- internal/mqtt/client.go
- internal/diskmanager/policy_usage.go
🔇 Additional comments (25)
internal/weather/icon_codes.go (1)
60-60
: LGTM! Idiomatic parameter declaration.The change to group parameters of the same type (
code, provider string
) follows Go's idiomatic style and improves readability.main.go (1)
31-33
: LGTM! Improved testability with mainWithExitCode.Good refactoring to separate the main logic into a testable function that returns an exit code.
internal/conf/locale.go (1)
85-85
: LGTM! Improved string comparison.Good change to use
strings.EqualFold
instead ofstrings.ToLower
. This is both more efficient (avoids allocation) and more correct for Unicode case-folding.cmd/benchmark/benchmark.go (1)
133-133
: LGTM! Named return values improve readability.The function signature change enhances code clarity by explicitly naming the return values as
rating
anddescription
.internal/httpcontroller/handlers/dashboard.go (1)
118-119
: LGTM! Performance improvements in note processing.The changes enhance performance by:
- Using an index-based loop to avoid copying each note
- Passing
hourlyCounts
by pointer to avoid copying the arrayAlso applies to: 124-124, 127-127
internal/httpcontroller/handlers/statistics.go (1)
61-62
: LGTM! Improved performance and date handling.The changes enhance the code by:
- Using an index-based loop to avoid copying each note
- Ensuring consistent date formatting using the parsed date's Format method
Also applies to: 69-69
internal/httpcontroller/template_functions.go (3)
231-231
: LGTM! Improved parameter declaration.The function signature is more concise by combining the related
sunrise
andsunset
parameters.
251-251
: LGTM! Performance improvement in hourly counts.Changed
element
parameter to pointer type to avoid copying the struct.
269-269
: LGTM! Performance improvement in range calculation.Changed
counts
parameter to pointer type to avoid copying the array.internal/birdweather/birdweather.go (2)
Line range hint
64-80
: LGTM! Named return values improve readability.The change to use named return values (
latitude, longitude float64
) makes the function's interface more self-documenting while preserving the correct logic for location randomization.
Line range hint
260-297
: LGTM! Using pointer parameter improves performance.The change to accept
*datastore.Note
instead ofdatastore.Note
is a good optimization that avoids unnecessary struct copying while maintaining the same functionality.internal/httpcontroller/handlers/detections.go (1)
247-280
: LGTM! Index-based iteration ensures consistent access.The change to use index-based iteration (
for i := range notes
) is a good practice here as it ensures consistent access to the notes slice while maintaining proper error handling and caching logic.internal/conf/utils.go (3)
97-97
: LGTM! Modern octal literal syntax.The change from
0755
to0o755
updates the code to use the modern Go octal literal syntax while maintaining the same permission value.
225-225
: LGTM! Simplified empty string check.The change from
len(retention) == 0
toretention == ""
is a more idiomatic way to check for empty strings in Go.
328-328
: LGTM! Named return values improve readability.The change to use named return values (
isAvailable bool, formats []string
) makes the function's interface more self-documenting.internal/analysis/realtime.go (3)
191-193
: LGTM! Simplified function signature.The removal of the unused
dataStore
parameter simplifies the function interface.
Line range hint
250-282
: LGTM! Improved separation of concerns.The removal of the
dataStore
parameter and the use of thediskmanager
package for cleanup operations improves the separation of concerns.
Line range hint
305-319
: LGTM! Index-based iteration ensures correct species name access.The change to use index-based iteration ensures that the correct scientific name is passed to the goroutine, avoiding potential issues with variable capture in the loop.
internal/datastore/interfaces.go (2)
261-261
: LGTM! Parameter consolidation in function signatures.The changes consolidate parameters of the same type into a single declaration, which improves readability and follows Go's idiomatic style.
Also applies to: 334-334, 381-381, 503-503
546-550
: LGTM! Named return values improve readability.The addition of named return values in
getHourRange
makes the code's intent clearer and self-documenting.internal/httpcontroller/handlers/settings.go (2)
69-69
: LGTM! Consistent pointer usage for settings.The changes align with the function signature updates, passing pointers to avoid unnecessary copying of the settings struct.
Also applies to: 79-79
Line range hint
579-600
: LGTM! Improved efficiency with pointer receivers.The changes to use pointer parameters for settings comparison functions:
- Avoid unnecessary copying of large structs
- Align with Go's best practices for handling large structs
- Maintain consistency across the codebase
Also applies to: 601-626
internal/httpcontroller/handlers/media.go (2)
388-388
: LGTM! Case-insensitive file extension comparison.Using
strings.EqualFold
for file extension comparison improves robustness by handling extensions regardless of case.
650-652
: LGTM! Improved Content-Disposition header formatting.The change to use
%q
for filename quoting in the Content-Disposition header:
- Ensures proper escaping of special characters
- Improves compatibility across different browsers
- Follows RFC 6266 specifications
internal/analysis/processor/actions.go (1)
215-215
: LGTM! Good optimization by passing the Note pointer.The change to pass a pointer to
Note
instead of the object itself is a good optimization that:
- Avoids unnecessary copying of the entire struct
- Aligns with the updated method signature in
birdweather.go
- Follows Go's idiomatic patterns for passing large structs
func validateMySQLConfig() error { | ||
// Add validation logic for MySQL configuration | ||
// Return an error if the configuration is invalid | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement proper MySQL configuration validation.
The validation function is empty and always returns nil, which could lead to runtime issues if the MySQL configuration is invalid. Since the struct has access to Settings
, we should validate the required MySQL configuration fields.
Consider implementing proper validation:
func validateMySQLConfig() error {
- // Add validation logic for MySQL configuration
- // Return an error if the configuration is invalid
- return nil
+ if store.Settings.Output.MySQL.Username == "" {
+ return fmt.Errorf("MySQL username is required")
+ }
+ if store.Settings.Output.MySQL.Password == "" {
+ return fmt.Errorf("MySQL password is required")
+ }
+ if store.Settings.Output.MySQL.Host == "" {
+ return fmt.Errorf("MySQL host is required")
+ }
+ if store.Settings.Output.MySQL.Port == "" {
+ return fmt.Errorf("MySQL port is required")
+ }
+ if store.Settings.Output.MySQL.Database == "" {
+ return fmt.Errorf("MySQL database name is required")
+ }
+ return nil
}
Committable suggestion skipped: line range outside the PR's diff.
// Add validation logic for MySQL configuration | ||
// Return an error if the configuration is invalid | ||
return nil | ||
} | ||
|
||
// InitializeDatabase sets up the MySQL database connection | ||
func (store *MySQLStore) Open() error { | ||
if err := validateMySQLConfig(store.Settings); err != nil { | ||
if err := validateMySQLConfig(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider making validateMySQLConfig a method.
Since the validation needs access to store.Settings (as seen in the Open method), it would be more idiomatic to make validateMySQLConfig a method of MySQLStore.
Apply this change:
-func validateMySQLConfig() error {
+func (store *MySQLStore) validateMySQLConfig() error {
And update the call site:
-if err := validateMySQLConfig(); err != nil {
+if err := store.validateMySQLConfig(); err != nil {
Committable suggestion skipped: line range outside the PR's diff.
// Add validation logic for SQLite configuration | ||
// Return an error if the configuration is invalid | ||
return nil | ||
} | ||
|
||
// InitializeDatabase sets up the SQLite database connection | ||
func (store *SQLiteStore) Open() error { | ||
if err := validateSQLiteConfig(store.Settings); err != nil { | ||
if err := validateSQLiteConfig(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider making validateSQLiteConfig a method.
Since the validation needs access to store.Settings (as seen in the Open method), it would be more idiomatic to make validateSQLiteConfig a method of SQLiteStore.
Apply this change:
-func validateSQLiteConfig() error {
+func (store *SQLiteStore) validateSQLiteConfig() error {
And update the call site:
-if err := validateSQLiteConfig(); err != nil {
+if err := store.validateSQLiteConfig(); err != nil {
Committable suggestion skipped: line range outside the PR's diff.
func validateSQLiteConfig() error { | ||
// Add validation logic for SQLite configuration | ||
// Return an error if the configuration is invalid | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement proper SQLite configuration validation.
The validation function is empty and always returns nil. Since SQLite requires a valid file path, this could lead to runtime issues if the path is invalid or inaccessible.
Consider implementing proper validation:
func validateSQLiteConfig() error {
- // Add validation logic for SQLite configuration
- // Return an error if the configuration is invalid
- return nil
+ if store.Settings.Output.SQLite.Path == "" {
+ return fmt.Errorf("SQLite database path is required")
+ }
+
+ // Validate that the directory exists or can be created
+ dir, _ := filepath.Split(store.Settings.Output.SQLite.Path)
+ basePath := conf.GetBasePath(dir)
+ if _, err := os.Stat(basePath); os.IsNotExist(err) {
+ return fmt.Errorf("directory %s does not exist", basePath)
+ }
+ return nil
}
Committable suggestion skipped: line range outside the PR's diff.
err = collectWindowsDiagnostics() | ||
case "darwin": | ||
err = collectMacOSDiagnostics(tmpDir) | ||
err = collectMacOSDiagnostics() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Restore the tmpDir
parameter for diagnostic collection functions
The removal of the tmpDir
parameter is premature. While the functions are currently stubs, their TODO comments indicate they need to save collected data to the temporary directory. The parameter should be restored to support the planned implementation.
internal/diagnostics/support_collect.go
: Restore thetmpDir
parameter to bothcollectWindowsDiagnostics()
andcollectMacOSDiagnostics()
functions
🔗 Analysis chain
Verify the implementation of diagnostic collection functions.
The removal of the tmpDir
parameter suggests that these functions now handle the temporary directory internally. Please ensure that:
- The implementations of
collectWindowsDiagnostics
andcollectMacOSDiagnostics
properly manage their temporary directories. - Any resources created in temporary directories are properly cleaned up.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of diagnostic collection functions.
# Search for the implementation of these functions
rg -A 10 "^func (collectWindowsDiagnostics|collectMacOSDiagnostics)"
# Look for temporary directory handling
rg "os\.(MkdirTemp|TempDir|RemoveAll)" internal/diagnostics/
Length of output: 2214
This pull request encompasses a comprehensive set of modifications across multiple files in the codebase, focusing primarily on refining function signatures, improving code clarity, and making syntax updates. The changes span various packages and include updates to method signatures, parameter handling, error management, and octal literal representations. While the modifications do not introduce significant new functionality, they enhance code readability, maintainability, and adhere to modern Go coding conventions.