Skip to content

Conversation

@StaticRocket
Copy link
Contributor

@StaticRocket StaticRocket commented Dec 11, 2025

Following the documentation the old imperative check was renamed to subject_imperative. Replace all old definitions with the new name to match documented behavior.

Summary by CodeRabbit

  • Refactor

    • Renamed a commit rule identifier from "imperative" to "subject_imperative"; validation behavior, messages, and outcomes remain unchanged.
  • Tests

    • Updated test cases and expectations to reference the renamed rule so validation results stay consistent and passing.
  • Chores

    • Simplified CI checkout steps to rely on default branch/commit selection.

✏️ Tip: You can customize this high-level summary in your review settings.

@StaticRocket StaticRocket requested a review from a team as a code owner December 11, 2025 23:42
@StaticRocket StaticRocket requested review from shenxianpeng and removed request for a team December 11, 2025 23:42
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 11, 2025

Walkthrough

Renamed the commit-check identifier from "imperative" to "subject_imperative" across rule definitions, validator mapping, requested checks, and tests; no validation logic, regexes, or messages were changed.

Changes

Cohort / File(s) Change Summary
Rule catalog
commit_check/rules_catalog.py
Renamed COMMIT_RULES entry key from "imperative" to "subject_imperative" (other fields unchanged).
Validation engine mapping
commit_check/engine.py
Updated ValidationEngine.VALIDATOR_MAP to use "subject_imperative" as the key for SubjectImperativeValidator.
Main/requested checks
commit_check/main.py
Replaced "imperative" with "subject_imperative" in the list of requested commit-checks when message checks are enabled.
Tests
tests/engine_test.py, tests/engine_comprehensive_test.py
Updated tests to use ValidationRule(check="subject_imperative") and to expect VALIDATOR_MAP mapping under "subject_imperative" instead of "imperative".
CI workflow
.github/workflows/main.yml
Removed explicit with: ref: ${{ github.head_ref }} from two actions/checkout steps (build and docs jobs).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify no remaining references to "imperative" in code, tests, docs.
  • Run full test suite to confirm validator lookups and CLI behavior.
  • Inspect changelog/docs for mentions of the old key.

Possibly related PRs

Suggested labels

bug

Poem

🐰 I hopped through lines with a curious twitch,
Swapped a name in the rules with a tiny switch,
"subject_imperative" now hopping in place,
I nibble a carrot and guard the code space,
Soft thumps of tests — a cozy, tidy stitch.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the primary change: renaming 'imperative' to 'subject_imperative' across the codebase to resolve inconsistency between documentation and implementation.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 14e883c and 19280cb.

📒 Files selected for processing (1)
  • .github/workflows/main.yml (0 hunks)
💤 Files with no reviewable changes (1)
  • .github/workflows/main.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
  • GitHub Check: install (3.14, macos-latest)
  • GitHub Check: install (3.14, ubuntu-24.04)
  • GitHub Check: install (3.13, windows-latest)
  • GitHub Check: install (3.11, windows-latest)
  • GitHub Check: install (3.13, ubuntu-24.04)
  • GitHub Check: install (3.14, windows-latest)
  • GitHub Check: install (3.10, macos-latest)
  • GitHub Check: install (3.11, macos-latest)
  • GitHub Check: install (3.10, ubuntu-24.04)
  • GitHub Check: install (3.12, windows-latest)
  • GitHub Check: install (3.9, ubuntu-24.04)
  • GitHub Check: install (3.9, macos-latest)
  • GitHub Check: install (3.12, macos-latest)
  • GitHub Check: install (3.12, ubuntu-24.04)
  • GitHub Check: install (3.11, ubuntu-24.04)
  • GitHub Check: install (3.9, windows-latest)
  • GitHub Check: install (3.10, windows-latest)
  • GitHub Check: Run benchmarks

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 and usage tips.

@netlify
Copy link

netlify bot commented Dec 11, 2025

Deploy Preview for commit-check ready!

Name Link
🔨 Latest commit 19280cb
🔍 Latest deploy log https://app.netlify.com/projects/commit-check/deploys/6941f006fef3c30008e23c1c
😎 Deploy Preview https://deploy-preview-329--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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c792143 and 98c420a.

📒 Files selected for processing (1)
  • commit_check/rules_catalog.py (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: commit-check/commit-check PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-03T10:28:06.793Z
Learning: Applies to .commit-check.yml : Maintain default validation rules for the tool in .commit-check.yml
📚 Learning: 2025-10-03T10:28:06.793Z
Learnt from: CR
Repo: commit-check/commit-check PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-03T10:28:06.793Z
Learning: Applies to .commit-check.yml : Maintain default validation rules for the tool in .commit-check.yml

Applied to files:

  • commit_check/rules_catalog.py
📚 Learning: 2025-10-03T10:28:06.793Z
Learnt from: CR
Repo: commit-check/commit-check PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-03T10:28:06.793Z
Learning: Applies to commit_check/commit.py : Enforce Conventional Commits format: type(scope): description

Applied to files:

  • commit_check/rules_catalog.py
📚 Learning: 2025-10-03T10:28:06.793Z
Learnt from: CR
Repo: commit-check/commit-check PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-03T10:28:06.793Z
Learning: Applies to commit_check/commit.py : Support breaking change notation with ! (e.g., feat!: breaking change)

Applied to files:

  • commit_check/rules_catalog.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Run benchmarks

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 11, 2025

CodSpeed Performance Report

Merging #329 will not alter performance

Comparing StaticRocket:bugfix/subject_imperative (19280cb) with main (22cf07a)

Summary

✅ 167 untouched
⏩ 85 skipped1

Footnotes

  1. 85 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.

@StaticRocket StaticRocket force-pushed the bugfix/subject_imperative branch from 98c420a to fab3e0a Compare December 12, 2025 00:05
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
commit_check/main.py (1)

147-166: Consider temporary backward-compat alias for legacy imperative configs.

If users still have configs referencing imperative (pre-migration), this rename can effectively disable the validator (unknown check → skipped). If you want smoother upgrades, consider accepting both keys for a release (e.g., request both in CLI and/or alias in the engine/catalog).

-                    "subject_imperative",
+                    "subject_imperative",
+                    "imperative",  # backward-compat alias (optional)
commit_check/engine.py (1)

522-540: Optional: keep imperative as an alias to avoid silent skips.

Given unknown checks are silently skipped, legacy imperative configs could unintentionally bypass validation. If backward compatibility is desired:

     VALIDATOR_MAP: Dict[str, Type[BaseValidator]] = {
         "message": CommitMessageValidator,
         "subject_capitalized": SubjectCapitalizationValidator,
         "subject_imperative": SubjectImperativeValidator,
+        "imperative": SubjectImperativeValidator,  # backward-compat alias (optional)
         "subject_max_length": SubjectLengthValidator,
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 98c420a and fab3e0a.

📒 Files selected for processing (4)
  • commit_check/engine.py (1 hunks)
  • commit_check/main.py (1 hunks)
  • commit_check/rules_catalog.py (1 hunks)
  • tests/engine_test.py (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • commit_check/rules_catalog.py
🧰 Additional context used
📓 Path-based instructions (2)
commit_check/main.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

commit_check/main.py: Expose CLI flags: --message, --branch, --help, --version, --config, --dry-run; support combining checks
Exit with code 0 on success and 1 on validation failure; emit colorized ASCII-art rejection on errors

Files:

  • commit_check/main.py
tests/**

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Ensure tests run via pytest -v and cover commit, branch, author, and CLI behaviors

Files:

  • tests/engine_test.py
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: commit-check/commit-check PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-03T10:28:06.793Z
Learning: Applies to .commit-check.yml : Maintain default validation rules for the tool in .commit-check.yml
📚 Learning: 2025-10-03T10:28:06.793Z
Learnt from: CR
Repo: commit-check/commit-check PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-03T10:28:06.793Z
Learning: Applies to commit_check/commit.py : Support breaking change notation with ! (e.g., feat!: breaking change)

Applied to files:

  • commit_check/main.py
  • tests/engine_test.py
📚 Learning: 2025-10-03T10:28:06.793Z
Learnt from: CR
Repo: commit-check/commit-check PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-03T10:28:06.793Z
Learning: Applies to commit_check/main.py : Expose CLI flags: --message, --branch, --help, --version, --config, --dry-run; support combining checks

Applied to files:

  • commit_check/main.py
📚 Learning: 2025-10-03T10:28:06.793Z
Learnt from: CR
Repo: commit-check/commit-check PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-03T10:28:06.793Z
Learning: Applies to commit_check/commit.py : Enforce Conventional Commits format: type(scope): description

Applied to files:

  • commit_check/main.py
📚 Learning: 2025-10-03T10:28:06.793Z
Learnt from: CR
Repo: commit-check/commit-check PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-03T10:28:06.793Z
Learning: Applies to commit_check/commit.py : Provide special handling for merge commit messages

Applied to files:

  • commit_check/main.py
📚 Learning: 2025-10-03T10:28:06.793Z
Learnt from: CR
Repo: commit-check/commit-check PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-03T10:28:06.793Z
Learning: Applies to commit_check/commit.py : Support the set of commit types: build, chore, ci, docs, feat, fix, perf, refactor, revert, style, test

Applied to files:

  • commit_check/main.py
🧬 Code graph analysis (1)
tests/engine_test.py (1)
commit_check/rule_builder.py (1)
  • ValidationRule (14-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Run benchmarks
🔇 Additional comments (3)
commit_check/main.py (1)

147-166: Good alignment: CLI now requests subject_imperative under --message.

This matches the PR objective (catalog/config key consistency), so the check can be disabled via config as expected. As per learnings, CLI exposure/combining-checks behavior remains intact.

commit_check/engine.py (1)

522-540: Validator map rename to subject_imperative is correct and consistent.

This completes the end-to-end rename (rules → engine → CLI/tests), enabling config-based disabling to work.

tests/engine_test.py (1)

811-864: Tests updated to exercise the renamed check key (subject_imperative).

These cases now correctly bind to ValidationEngine.VALIDATOR_MAP["subject_imperative"] and validate the same behavior as before.

@StaticRocket
Copy link
Contributor Author

You know what, this cannot be toggled because the config logic right now doesn't match the catalog. Seems like everything else is using the old "imperative" definition, but it is a little weird how little the catalog is actually used right now. Most of the test files seem to define their own version of the check locally, which is still using the old name.

Do you want to update the config structure to match current tests or adjust all the existing references to match the current documentation?

@StaticRocket StaticRocket force-pushed the bugfix/subject_imperative branch from fab3e0a to ee940f6 Compare December 12, 2025 00:23
@StaticRocket StaticRocket changed the title fix(subject_imperative): catalog entry fix(subject_imperative): mixed definitions Dec 12, 2025
@shenxianpeng
Copy link
Contributor

Thank you for raising the PR. I think it would be better to rename all existing imperative checks to subject_imperative to align with the documentation. Could you also help fix the CI failures?

@shenxianpeng shenxianpeng force-pushed the bugfix/subject_imperative branch from ee940f6 to bcbe201 Compare December 15, 2025 09:59
Following the documentation the old imperative check was renamed to
subject_imperative. Replace all old definitions with the new name to
match documented behavior.

Fixes: 9f90bbd ("feat: Breaking Changes & Configuration Migration (v2 version) (commit-check#280)")
Signed-off-by: Randolph Sapp <rs@ti.com>
@StaticRocket StaticRocket force-pushed the bugfix/subject_imperative branch from bcbe201 to 14e883c Compare December 16, 2025 23:40
@StaticRocket
Copy link
Contributor Author

Sure, I'll take a look. Not sure how to address that reported performance regression though. Seems like a false positive or something.

Ref pinning will incorrectly try to check out the branch name on the
project origin, not the pull request origin.

Fixes: 6a7a35c ("feat: inital commit")
Signed-off-by: Randolph Sapp <rs@ti.com>
@sonarqubecloud
Copy link

@codecov
Copy link

codecov bot commented Dec 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.75%. Comparing base (22cf07a) to head (19280cb).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #329      +/-   ##
==========================================
+ Coverage   87.46%   87.75%   +0.28%     
==========================================
  Files           8        8              
  Lines         694      694              
==========================================
+ Hits          607      609       +2     
+ Misses         87       85       -2     

☔ View full report in Codecov by Sentry.
📢 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.

@StaticRocket
Copy link
Contributor Author

@shenxianpeng was that all that you needed addressed?

@shenxianpeng
Copy link
Contributor

The performance regression does not need to be addressed, as it may be a false positive.

Copy link
Contributor

@shenxianpeng shenxianpeng left a comment

Choose a reason for hiding this comment

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

Thank you

@shenxianpeng shenxianpeng merged commit d356a6e into commit-check:main Dec 17, 2025
33 checks passed
@shenxianpeng shenxianpeng added the bug Something isn't working label Dec 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants