Skip to content

chore: fix SonarQube code smells - #480

Merged
shenxianpeng merged 2 commits into
mainfrom
chore/fix-sonarqube-code-smells
Jul 24, 2026
Merged

chore: fix SonarQube code smells#480
shenxianpeng merged 2 commits into
mainfrom
chore/fix-sonarqube-code-smells

Conversation

@shenxianpeng

@shenxianpeng shenxianpeng commented Jul 24, 2026

Copy link
Copy Markdown
Member

Summary

Fixes 3 categories of SonarQube-reported code smells (58 total issues → expect ~54/58 to resolve after this PR).

Changes

  1. commit_check/engine.py:296 — Remove unnecessary outer non-capturing group in regex. The (?:...) wrapper had no quantifier or alternation — purely decorative. Same behavior, cleaner pattern.

  2. tests/config_test.py:311 — Add match="[Ee]xpected" to pytest.raises(Exception) so the assertion verifies we caught the right TOML parsing error, not any random Exception.

  3. tests/main_test.py (55 lines) — Replace all sys.argv = ... direct assignments with monkeypatch.setattr("sys.argv", ...). This prevents global state leaking between tests and automatically restores the original value after each test.

Verification

  • pytest tests/main_test.py -v — ✅ 60 passed
  • pytest tests/config_test.py tests/config_merger_test.py -v — ✅ 87 passed
  • pytest --ignore=tests/engine_test.py -v — ✅ 314 passed (the 1 failure in engine_test.py::test_default_signoff_skips_ignored_author is pre-existing on main)

SonarQube Impact

Issue File Status
Unwrap unnecessarily grouped subpattern engine.py:296 Fixed
Assertion too broad config_test.py:311 Fixed
Use monkeypatch (55 instances) main_test.py Fixed
Refactor exception test config_merger_test.py:378 Skipped — already uses specific FileNotFoundError

Summary by CodeRabbit

  • Bug Fixes

    • Improved conventional commit subject parsing for more accurate capitalization validation.
    • Invalid TOML configuration files now report clearer parsing errors.
  • Tests

    • Expanded coverage for commit messages from files, JSON output, banners, compact output, force-push settings, and configuration failures.
    • Improved validation of command-line behavior and error messages.

…gv usage

- Remove unnecessary non-capturing subpattern in regex (engine.py:296)
- Add match parameter to broad pytest.raises(Exception) in config_test.py
- Replace sys.argv = ... with monkeypatch.setattr() in main_test.py (55 instances)
  to avoid modifying global state, following SonarQube recommendation
@shenxianpeng
shenxianpeng requested a review from a team as a code owner July 24, 2026 20:19
@netlify

netlify Bot commented Jul 24, 2026

Copy link
Copy Markdown

Deploy Preview for commit-check ready!

Name Link
🔨 Latest commit acf0266
🔍 Latest deploy log https://app.netlify.com/projects/commit-check/deploys/6a63cd68af4f9100088990b8
😎 Deploy Preview https://deploy-preview-480--commit-check.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions github-actions Bot added chore tests Add test related changes labels Jul 24, 2026
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The subject capitalization regex now extracts conventional-commit descriptions differently. Tests tighten invalid TOML assertions and standardize CLI argument setup with monkeypatch, covering validation, output formats, configuration, positional arguments, and force-push behavior.

Changes

Validation and CLI tests

Layer / File(s) Summary
Subject and configuration validation
commit_check/engine.py, tests/config_test.py
The subject capitalization parsing pattern changed, and invalid TOML tests now verify a parsing-style error message.
CLI test isolation
tests/main_test.py
Core, edge-case, integration, environment, configuration-priority, and positional-argument tests now use monkeypatch.setattr for sys.argv.
Output and force-push scenarios
tests/main_test.py
JSON, banner, compact-output, and no-force-push tests now use the standardized CLI setup while retaining their behavioral assertions.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested labels: developer

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately describes the main goal of the PR: fixing SonarQube code smells.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/fix-sonarqube-code-smells

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.28%. Comparing base (911de5e) to head (acf0266).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #480      +/-   ##
==========================================
+ Coverage   97.20%   97.28%   +0.08%     
==========================================
  Files          12       12              
  Lines        1179     1179              
==========================================
+ Hits         1146     1147       +1     
+ Misses         33       32       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/config_test.py (1)

311-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use a raw string for the pytest regex.

Ruff reports RUF043 because the match= pattern contains regex metacharacters but is not written as a raw string. Use match=r"[Ee]xpected" to preserve behavior and clear the warning.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/config_test.py` around lines 311 - 313, Update the pytest.raises
assertion in the TOML parsing test to pass the regex pattern as a raw string,
using the existing "[Ee]xpected" pattern and preserving its matching behavior.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@commit_check/engine.py`:
- Line 296: Update the subject-matching regex in the relevant validation flow to
treat `!:` as a single breaking-commit delimiter, requiring the colon after an
optional exclamation mark so `feat!: Add feature` captures the message correctly
while preserving existing type and scope formats. Add a regression case in the
tests around the engine validation to confirm this notation with a capitalized
subject is accepted.

---

Nitpick comments:
In `@tests/config_test.py`:
- Around line 311-313: Update the pytest.raises assertion in the TOML parsing
test to pass the regex pattern as a raw string, using the existing "[Ee]xpected"
pattern and preserving its matching behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 51b646c5-b97e-48a8-81ec-bc0f103206d7

📥 Commits

Reviewing files that changed from the base of the PR and between 911de5e and 9d534cb.

📒 Files selected for processing (3)
  • commit_check/engine.py
  • tests/config_test.py
  • tests/main_test.py

Comment thread commit_check/engine.py Outdated
@shenxianpeng shenxianpeng removed the tests Add test related changes label Jul 24, 2026
@codspeed-hq

codspeed-hq Bot commented Jul 24, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by ×2.8

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
✅ 379 untouched benchmarks
⏩ 114 skipped benchmarks1

Performance Changes

Benchmark BASE HEAD Efficiency
test_empty_message_passes 2,078 µs 748.4 µs ×2.8

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.


Comparing chore/fix-sonarqube-code-smells (acf0266) with main (911de5e)

Open in CodSpeed

Footnotes

  1. 114 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Comment thread commit_check/engine.py Outdated
Co-authored-by: Xianpeng Shen <xianpeng.shen@gmail.com>
@github-actions github-actions Bot added the tests Add test related changes label Jul 24, 2026
@shenxianpeng shenxianpeng removed the tests Add test related changes label Jul 24, 2026
@sonarqubecloud

Copy link
Copy Markdown

@shenxianpeng
shenxianpeng merged commit bd890f1 into main Jul 24, 2026
33 checks passed
@shenxianpeng
shenxianpeng deleted the chore/fix-sonarqube-code-smells branch July 24, 2026 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant