Skip to content

Conversation

@devin-ai-integration
Copy link

Summary

This PR addresses SonarQube High/Critical severity issues identified in the github-mcp-server repository by fixing code quality issues in two categories:

  1. String literal duplication (rule go:S1192) in pkg/github/discussions.go
  2. Cognitive complexity (rule go:S3776) in pkg/github/repositories.go

Changes

1. Fix String Literal Duplication in discussions.go

Added constants at the package level to eliminate repeated string literals:

  • descRepositoryOwner = "Repository owner" (4 occurrences)
  • descRepositoryName = "Repository name" (4 occurrences)
  • errGitHubGQLClient = "failed to get GitHub GQL client: %v" (4 occurrences)
  • discussionCategoryLabelFmt = "category:%s" (3 occurrences)

These constants are now used consistently across 4 functions: ListDiscussions, GetDiscussion, GetDiscussionComments, and ListDiscussionCategories.

2. Reduce Cognitive Complexity in repositories.go

Refactored the ListCommits function to reduce cognitive complexity from 23 to well below the threshold of 15:

  • Created listCommitsParams struct to encapsulate function parameters
  • Extracted parameter parsing logic into parseListCommitsParams helper function
  • This separates validation concerns from business logic, making the code more maintainable

Review Checklist

Critical items to verify:

  1. ✅ All string constant substitutions maintain identical behavior
  2. ✅ Parameter parsing extraction preserves exact error handling paths
  3. ✅ The PaginationParams type name is correctly capitalized (was initially lowercase, fixed during development)
  4. ✅ All tests pass and lint is clean

What to look for:

  • Confirm that error messages returned to users are unchanged
  • Verify that the parameter parsing in parseListCommitsParams matches the original inline logic exactly
  • Check that the struct field accesses (params.owner, params.repo, etc.) are correct

Testing

  • ./script/lint passes with 0 issues
  • ./script/test passes all test suites

Tradeoffs

  • Constants scope: The new constants are package-private (lowercase names), which is appropriate for this use case. If similar patterns emerge in other packages, we may want to consider broader scoping.
  • Extraction granularity: Only extracted parameter parsing, not the full function body, to keep changes minimal and focused on reducing complexity.

Link to Devin run: https://app.devin.ai/sessions/20c43147c2b74e5f9a077535303e2d5b
Requested by: Shawn Azman (shawn@cognition.ai) (@ShawnAzman)

- Fix string literal duplication in pkg/github/discussions.go
  - Define constants for repeated strings: Repository owner, Repository name, error messages, and category label format
  - Replace 4 occurrences of 'Repository owner' with descRepositoryOwner constant
  - Replace 4 occurrences of 'Repository name' with descRepositoryName constant
  - Replace 4 occurrences of error message with errGitHubGQLClient constant
  - Replace 3 occurrences of 'category:%s' with discussionCategoryLabelFmt constant

- Fix cognitive complexity in pkg/github/repositories.go
  - Extract parameter parsing from ListCommits function into parseListCommitsParams helper
  - Reduce cognitive complexity from 23 to well below the threshold of 15
  - Create listCommitsParams struct to encapsulate all function parameters
@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

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
5.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

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.

2 participants