fix(cli): report error message when type scope or body empty #445
Conversation
This commit to fix follow rules report error message:
- type
```shell
$> echo -n "add new flag" | commitlint
type is longer than 4 characters
```
Config rules see below:
```yaml
rules:
type-max-length:
level: error
length: 4
```
- scope
```shell
$> echo -n "feat: add new flag" | commitlint
scope is longer than 5 characters
```
Config rules see below:
```yaml
rules:
scope-max-length:
level: error
length: 5
```
- body
```shell
$> echo -n "feat(cli): add new flag" | commitlint
body is longer than 72 characters
```
Config rules see below
```yaml
rules:
body-empty:
level: ignore
body-max-length:
level: error
length: 72
```
Signed-off-by: xieyongbin <xieyongbin@126.com>
WalkthroughRefactors three rule validators to replace match on optional fields with if-let Some(...) guards; preserves length-check logic and violation construction; no public API changes. (49 words) Changes
Sequence Diagram(s)(omitted — changes are localized control-flow simplifications without new external interactions) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Out-of-scope changesNo out-of-scope functional changes detected. Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cli/src/rule/type_max_length.rs (1)
57-75: Tests: add coverage for None/empty and equality-at-limitCurrent tests don’t prove the intended fix (no violation when type is None/empty) and don’t cover the equality boundary. Please add:
Example tests to add:
#[test] fn test_type_none_is_ignored_by_max_length() { let rule = TypeMaxLength { length: 4, ..Default::default() }; let message = Message { body: None, description: None, footers: None, r#type: None, raw: "".to_string(), scope: None, subject: None, }; assert!(rule.validate(&message).is_none()); } #[test] fn test_type_empty_string_is_ignored_by_max_length_even_when_limit_zero() { let rule = TypeMaxLength { length: 0, ..Default::default() }; let message = Message { body: None, description: None, footers: None, r#type: Some("".to_string()), raw: "".to_string(), scope: None, subject: None, }; assert!(rule.validate(&message).is_none()); } #[test] fn test_type_equal_to_max_is_ok() { let rule = TypeMaxLength { length: 4, ..Default::default() }; let message = Message { body: None, description: None, footers: None, r#type: Some("feat".to_string()), raw: "feat: x".to_string(), scope: None, subject: None, }; assert!(rule.validate(&message).is_none()); }cli/src/rule/scope_max_length.rs (1)
57-75: Tests: add coverage for None/empty and equality-at-limitMissing assertions for the scope being None/empty and equals-to-limit.
Example tests to add:
#[test] fn test_scope_none_is_ignored_by_max_length() { let rule = ScopeMaxLength { length: 5, ..Default::default() }; let message = Message { body: None, description: None, footers: None, r#type: Some("feat".to_string()), raw: "feat: x".to_string(), scope: None, subject: None, }; assert!(rule.validate(&message).is_none()); } #[test] fn test_scope_empty_string_is_ignored_by_max_length_even_when_limit_zero() { let rule = ScopeMaxLength { length: 0, ..Default::default() }; let message = Message { body: None, description: None, footers: None, r#type: Some("feat".to_string()), raw: "feat: x".to_string(), scope: Some("".to_string()), subject: None, }; assert!(rule.validate(&message).is_none()); } #[test] fn test_scope_equal_to_max_is_ok() { let rule = ScopeMaxLength { length: 5, ..Default::default() }; let message = Message { body: None, description: None, footers: None, r#type: Some("feat".to_string()), raw: "feat(scope): x".to_string(), scope: Some("scope".to_string()), subject: None, }; assert!(rule.validate(&message).is_none()); }cli/src/rule/body_max_length.rs (1)
57-77: Tests: add coverage for None/empty and equality-at-limitTo prove the bugfix and guard the boundary:
Example tests to add:
#[test] fn test_body_none_is_ignored_by_max_length() { let rule = BodyMaxLength { length: 72, ..Default::default() }; let message = Message { body: None, description: None, footers: None, r#type: Some("feat".to_string()), raw: "feat: x".to_string(), scope: None, subject: None, }; assert!(rule.validate(&message).is_none()); } #[test] fn test_body_empty_string_is_ignored_by_max_length_even_when_limit_zero() { let rule = BodyMaxLength { length: 0, ..Default::default() }; let message = Message { body: Some("".to_string()), description: None, footers: None, r#type: Some("feat".to_string()), raw: "feat: x".to_string(), scope: None, subject: None, }; assert!(rule.validate(&message).is_none()); } #[test] fn test_body_equal_to_max_is_ok() { let body = "Hello"; // length 5 let rule = BodyMaxLength { length: 5, ..Default::default() }; let message = Message { body: Some(body.to_string()), description: None, footers: None, r#type: Some("feat".to_string()), raw: format!("feat: {}\n\n{}", "x", body), scope: None, subject: None, }; assert!(rule.validate(&message).is_none()); }
🧹 Nitpick comments (5)
cli/src/rule/type_max_length.rs (2)
30-35: Optional: Count “characters” not bytes, or reword the messagelen() counts bytes, while the message says “characters.” If non-ASCII commit messages are allowed, prefer .chars().count() to match the wording; otherwise, reword to “bytes”.
If you choose to align with characters:
- if t.len() > self.length { + if t.chars().count() > self.length {Note: This change impacts performance minimally for short strings and avoids misreporting lengths for Unicode.
6-6: Docstring nit: wrong rule nameThe comment says “description-max-length” but this struct implements type-max-length.
Apply this diff:
-/// TypeMaxLength represents the description-max-length rule. +/// TypeMaxLength represents the type-max-length rule.cli/src/rule/scope_max_length.rs (2)
30-35: Optional: Count “characters” not bytes, or reword the messageAlign the implementation with the message wording “characters”.
If aligning:
- if scope.len() > self.length { + if scope.chars().count() > self.length {
6-6: Docstring nit: wrong rule nameThe comment mentions “description-max-length” instead of scope-max-length.
Apply this diff:
-/// ScopeMaxLength represents the description-max-length rule. +/// ScopeMaxLength represents the scope-max-length rule.cli/src/rule/body_max_length.rs (1)
30-35: Optional: Count “characters” not bytes, or reword the messagelen() counts bytes; message says “characters”.
If aligning:
- if body.len() > self.length { + if body.chars().count() > self.length {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
cli/src/rule/body_max_length.rs(1 hunks)cli/src/rule/scope_max_length.rs(1 hunks)cli/src/rule/type_max_length.rs(1 hunks)
| if let Some(body) = &message.body { | ||
| if body.len() >= self.length { | ||
| return Some(Violation { | ||
| level: self.level.unwrap_or(Self::LEVEL), | ||
| message: self.message(message), | ||
| }) | ||
| }); |
There was a problem hiding this comment.
Fix boundary condition: use strict “greater than” for max-length checks
Equality with the limit should pass. Using >= causes false positives and can still flag an empty body when limit is 0.
Apply this diff:
- if let Some(body) = &message.body {
- if body.len() >= self.length {
+ if let Some(body) = &message.body {
+ if body.len() > self.length {
return Some(Violation {
level: self.level.unwrap_or(Self::LEVEL),
message: self.message(message),
});
}
}🤖 Prompt for AI Agents
In cli/src/rule/body_max_length.rs around lines 30 to 35, the max-length check
uses a non-strict comparison (body.len() >= self.length) which incorrectly flags
messages whose body length equals the limit (including empty body when limit is
0); change the comparison to a strict greater-than (body.len() > self.length) so
only lengths strictly greater than the configured max produce a Violation.
| if let Some(scope) = &message.scope { | ||
| if scope.len() >= self.length { | ||
| return Some(Violation { | ||
| level: self.level.unwrap_or(Self::LEVEL), | ||
| message: self.message(message), | ||
| }) | ||
| }); |
There was a problem hiding this comment.
Fix boundary condition: use strict “greater than” for max-length checks
Same issue as type: equality with the limit should not violate, and length 0 shouldn’t violate when max is 0.
Apply this diff:
- if let Some(scope) = &message.scope {
- if scope.len() >= self.length {
+ if let Some(scope) = &message.scope {
+ if scope.len() > self.length {
return Some(Violation {
level: self.level.unwrap_or(Self::LEVEL),
message: self.message(message),
});
}
}🤖 Prompt for AI Agents
In cli/src/rule/scope_max_length.rs around lines 30 to 35, the max-length check
uses >= which incorrectly flags scopes equal to the limit (including the 0
case); change the condition to use strict greater-than so it only violates when
scope.len() > self.length (i.e., replace `if scope.len() >= self.length` with
`if scope.len() > self.length`) and keep the rest of the Violation construction
unchanged.
| if let Some(t) = &message.r#type { | ||
| if t.len() >= self.length { | ||
| return Some(Violation { | ||
| level: self.level.unwrap_or(Self::LEVEL), | ||
| message: self.message(message), | ||
| }) | ||
| }); |
There was a problem hiding this comment.
Fix boundary condition: use strict “greater than” for max-length checks
Max-length rules should pass when the length equals the limit. Using >= incorrectly flags values exactly at the configured maximum and also causes empty strings to violate when length is 0. Switch to >.
Apply this diff:
- if let Some(t) = &message.r#type {
- if t.len() >= self.length {
+ if let Some(t) = &message.r#type {
+ if t.len() > self.length {
return Some(Violation {
level: self.level.unwrap_or(Self::LEVEL),
message: self.message(message),
});
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(t) = &message.r#type { | |
| if t.len() >= self.length { | |
| return Some(Violation { | |
| level: self.level.unwrap_or(Self::LEVEL), | |
| message: self.message(message), | |
| }) | |
| }); | |
| if let Some(t) = &message.r#type { | |
| if t.len() > self.length { | |
| return Some(Violation { | |
| level: self.level.unwrap_or(Self::LEVEL), | |
| message: self.message(message), | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
In cli/src/rule/type_max_length.rs around lines 30 to 35, the max-length check
currently uses >= which incorrectly flags values exactly equal to the limit (and
empty strings when limit is 0); change the conditional to use > so only lengths
strictly greater than self.length produce a Violation, keeping the rest of the
Violation construction intact.
KeisukeYamashita
changed the title
KeisukeYamashita
merged commit 428d4c7
into
KeisukeYamashita:main
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/src/rule/scope_max_length.rs (1)
30-36: Fix boundary condition: use strict “greater than” for max-length checksMax length should be inclusive; equality with the limit must not violate. Using
>=incorrectly flags scopes whose length equals the configured maximum (including 0). Switch to>to align with the rule’s intent and the linked issues’ objectives.Apply this diff:
- if let Some(scope) = &message.scope { - if scope.len() >= self.length { + if let Some(scope) = &message.scope { + if scope.len() > self.length { return Some(Violation { level: self.level.unwrap_or(Self::LEVEL), message: self.message(message), }); } }Run this script to ensure no remaining
>= self.lengthcomparisons exist across max-length rules:#!/bin/bash # Verify all max-length rules use strict `>` rg -nP -C2 --glob 'cli/src/rule/**' $'len\\(\\)\\s*>=\\s*self\\.length|\\.len\\(\\)\\s*>=\\s*self\\.length'
🧹 Nitpick comments (3)
cli/src/rule/scope_max_length.rs (3)
6-6: Correct copy/paste in doc commentUpdate the docstring to reference the correct rule name.
-/// ScopeMaxLength represents the description-max-length rule. +/// ScopeMaxLength represents the scope-max-length rule.
95-118: Add equality and empty-string tests; rename test for clarityStrengthen tests to lock the intended semantics and avoid regressions:
- Add a test where scope length equals the limit (no violation).
- Add a test where scope is the empty string (no violation; emptiness is handled by scope-empty).
- Rename
test_short_scopeto better reflect it verifies “exceeds limit”.Apply this diff:
- fn test_short_scope() { + fn test_scope_exceeds_limit() { let rule = ScopeMaxLength { length: 3, // Short length for testing ..Default::default() }; let message = Message { body: None, description: Some("feat(scope): I'm long description".to_string()), footers: None, r#type: Some("feat".to_string()), raw: "feat(scope): I'm long description".to_string(), scope: Some("scope".to_string()), subject: None, }; let violation = rule.validate(&message); assert!(violation.is_some()); assert_eq!(violation.clone().unwrap().level, Level::Error); assert_eq!( violation.unwrap().message, format!("scope is longer than {} characters", rule.length) ); } + + #[test] + fn test_scope_equal_to_limit_no_violation() { + let rule = ScopeMaxLength { + length: 5, + ..Default::default() + }; + let message = Message { + body: None, + description: Some("desc".to_string()), + footers: None, + r#type: Some("feat".to_string()), + raw: "feat(scope): desc".to_string(), + scope: Some("scope".to_string()), // len == 5 + subject: Some("feat(scope): desc".to_string()), + }; + assert!(rule.validate(&message).is_none()); + } + + #[test] + fn test_empty_scope_string_no_violation() { + let rule = ScopeMaxLength { + length: 5, + ..Default::default() + }; + let message = Message { + body: None, + description: Some("desc".to_string()), + footers: None, + r#type: Some("feat".to_string()), + raw: "feat(): desc".to_string(), + scope: Some("".to_string()), + subject: Some("feat(): desc".to_string()), + }; + assert!(rule.validate(&message).is_none()); + }I can mirror these tests in type/body max-length modules if desired.
31-31: Optional: count user-perceived characters, not bytes
String::len()counts bytes; multi-byte Unicode scopes (e.g., emoji) will be penalized earlier than intended. If international input is common, consider counting grapheme clusters.Example (behind an optional feature):
- if scope.len() > self.length { + #[cfg(feature = "unicode_graphemes")] + { + use unicode_segmentation::UnicodeSegmentation; + if scope.graphemes(true).count() > self.length { + return Some(Violation { + level: self.level.unwrap_or(Self::LEVEL), + message: self.message(message), + }); + } + } + #[cfg(not(feature = "unicode_graphemes"))] + if scope.len() > self.length { return Some(Violation { level: self.level.unwrap_or(Self::LEVEL), message: self.message(message), }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
cli/src/rule/body_max_length.rs(1 hunks)cli/src/rule/scope_max_length.rs(1 hunks)cli/src/rule/type_max_length.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cli/src/rule/type_max_length.rs
- cli/src/rule/body_max_length.rs
🧰 Additional context used
🧬 Code graph analysis (1)
cli/src/rule/scope_max_length.rs (1)
cli/src/rule.rs (1)
message(220-220)
Why
I've tested the tool and plan to implement it in our company.
However there are some logical errors which blocked us to use it.
This commit to fix
(type|scope|body)_max-lengthrules report incorrect message.fix #440
fix #441
fix #442
Signed-off-by: xieyongbin xieyongbin@126.com
Summary by CodeRabbit