Skip to content

fix: resolve SonarQube S1192 and S3776 High severity issues in discussions.go - #147

Open
devin-ai-integration[bot] wants to merge 2 commits into
mainfrom
devin/1779230148-fix-sonarqube-discussions
Open

devin-ai-integration[bot] wants to merge 2 commits into
mainfrom
devin/1779230148-fix-sonarqube-discussions

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented May 19, 2026

Copy link
Copy Markdown

Closes: N/A (SonarQube-driven code quality improvements)

Summary

Addresses two HIGH severity SonarQube findings in pkg/github/discussions.go:

S1192 — String literals should not be duplicated

Extracted four duplicated string literals into package-level constants:

  • descRepoOwner ("Repository owner") — was duplicated 4×
  • descRepoName ("Repository name") — was duplicated 4×
  • errGQLClientFmt ("failed to get GitHub GQL client: %v") — was duplicated 4×
  • categoryLabelFmt ("category:%s") — was duplicated 3×

S3776 — Cognitive Complexity of functions should not be too high

Reduced the cognitive complexity of ListDiscussions from 27 → ~8 (allowed max: 15):

  • Extracted a shared discussionNode struct to eliminate inline struct duplication
  • Extracted mapDiscussionNodesToIssues() helper to consolidate the node→Issue mapping logic that was copy-pasted in both branches
  • Extracted listDiscussionsWithCategory() and listDiscussionsAll() helpers to move the two GraphQL query branches out of the main handler
  • Flattened the main handler to a simple branch + shared error/marshal path

Tradeoffs

  • Added two unexported helper functions and one unexported struct — minimal API surface increase, but significantly cleaner handler body.
  • The helpers are specific to discussions.go and not exported, keeping the change scoped.

Alternatives

  • Could have used a single helper with a nullable category param, but that would require the GraphQL query struct to always include the categoryId variable, which isn't valid for the no-filter case due to how githubv4 constructs queries.

Verification

  • go build ./... passes
  • go vet ./... passes
  • All existing discussion tests pass (Test_ListDiscussions, Test_GetDiscussion, Test_GetDiscussionComments, Test_ListDiscussionCategories)

Link to Devin session: https://app.devin.ai/sessions/975cc3c24a394a1397adb7d1362fe6ae
Requested by: @VedantKh


Devin Review

Status Commit
⚪ Not started

Run Devin Review

💡 Connect your GitHub account to enable automatic code reviews.

Open in Devin Review (Staging)

S1192 - Extract duplicated string literals to package-level constants:
  - descRepoOwner, descRepoName, errGQLClientFmt, categoryLabelFmt

S3776 - Reduce cognitive complexity of ListDiscussions from 27 to <=15:
  - Extract discussionNode struct and mapDiscussionNodesToIssues helper
  - Extract listDiscussionsWithCategory and listDiscussionsAll helpers
  - Flatten branching in the main handler

Co-Authored-By: Vedant Khanna <vedantkhanna@gmail.com>
@devin-ai-integration

Copy link
Copy Markdown
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

devin-ai-integration[bot]

This comment was marked as resolved.

Use var declaration instead of make() so json.Marshal produces
null (not []) when there are no discussions, matching original behavior.

Co-Authored-By: Vedant Khanna <vedantkhanna@gmail.com>
@sonarqubecloud

Copy link
Copy Markdown

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