Skip to content

Conversation

@devin-ai-integration
Copy link

Summary

Fixes two critical SonarQube code quality issues identified in the codebase:

  1. go:S1192: String literal duplication - "failed to read response body: %w" was duplicated 30+ times
  2. go:S3776: High cognitive complexity in GetCodeScanningAlert function (complexity 17, threshold 15)

Changes Made

1. String Duplication Fix (go:S1192)

  • Created pkg/github/constants.go with ErrFailedToReadResponseBody constant
  • Replaced 30+ occurrences of the duplicated string literal across 9 files:
    • code_scanning.go (2 occurrences)
    • issues.go (6 occurrences)
    • notifications.go (4 occurrences)
    • pullrequests.go (15 occurrences)
    • repositories.go (12 occurrences)
    • repository_resource.go (1 occurrence)
    • search.go (3 occurrences)
    • search_utils.go (1 occurrence, special handling)
    • secret_scanning.go (2 occurrences)

2. Cognitive Complexity Fix (go:S3776)

  • Added validateResponseAndReadBody helper function in code_scanning.go
  • Refactored GetCodeScanningAlert to use the helper, reducing complexity from 17 to below 15
  • Applied the same pattern to ListCodeScanningAlerts for consistency
  • The helper consolidates response status checking and error body reading into a single reusable function

Testing

  • ✅ Lint: ./script/lint - 0 issues
  • ✅ Tests: ./script/test - All tests passing
  • ⚠️ Note: Changes not tested with live GitHub API calls

Review Checklist

Critical areas to review:

  1. Helper function logic (lines 17-26 in code_scanning.go): Verify the dual return value handling is correct
  2. String concatenation in search_utils.go (line 77): Uses "%s: "+ErrFailedToReadResponseBody pattern - confirm this is acceptable
  3. Error message formatting: Verify that fmt.Errorf(ErrFailedToReadResponseBody, err) correctly formats the error with %w
  4. Consistency: All 46 replacements use the constant correctly across different contexts

Nice to have:

  • Re-run SonarQube analysis to confirm both issues are resolved
  • Integration test with actual GitHub API to verify error messages format correctly

Tradeoffs

  • String concatenation in search_utils.go: Used "%s: "+ErrFailedToReadResponseBody instead of extracting a separate constant. This was chosen to maintain the error prefix pattern while still using the constant.
  • Helper function scope: Added to code_scanning.go rather than a shared utilities file. Could be moved to a common location if other files would benefit from it.

Link to Devin run

https://app.devin.ai/sessions/edb413c51d3f4c0ab35658c626c0c9b1

Requested by: Zoheb Munshi (@zohebmunshi)

- Create constants.go with ErrFailedToReadResponseBody constant
- Replace all 30+ occurrences of 'failed to read response body: %w' with constant
- Refactor GetCodeScanningAlert to reduce cognitive complexity from 17 to below 15
- Extract validateResponseAndReadBody helper function for cleaner error handling
- Apply same pattern to ListCodeScanningAlerts for consistency

Fixes go:S1192 (String literals should not be duplicated) and go:S3776 (Cognitive Complexity)

Link to Devin run: https://app.devin.ai/sessions/edb413c51d3f4c0ab35658c626c0c9b1
Requested by: Zoheb Munshi (@zohebmunshi)

Co-Authored-By: Zoheb Munshi <zohebmunshi@gmail.com>
@devin-ai-integration
Copy link
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant