-
-
Notifications
You must be signed in to change notification settings - Fork 8
fix(subject_imperative): mixed definitions #329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(subject_imperative): mixed definitions #329
Conversation
WalkthroughRenamed 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
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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)
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. Comment |
✅ Deploy Preview for commit-check ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this 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
📒 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 Performance ReportMerging #329 will not alter performanceComparing Summary
Footnotes
|
98c420a to
fab3e0a
Compare
There was a problem hiding this 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 legacyimperativeconfigs.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: keepimperativeas an alias to avoid silent skips.Given unknown checks are silently skipped, legacy
imperativeconfigs 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
📒 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.pytests/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 requestssubject_imperativeunder--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 tosubject_imperativeis 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.
|
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? |
fab3e0a to
ee940f6
Compare
|
Thank you for raising the PR. I think it would be better to rename all existing |
ee940f6 to
bcbe201
Compare
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>
bcbe201 to
14e883c
Compare
|
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>
|
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
|
@shenxianpeng was that all that you needed addressed? |
|
The performance regression does not need to be addressed, as it may be a false positive. |
shenxianpeng
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you



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
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.