feat(cli): generate Json Schema #389
Conversation
|
Warning Rate limit exceeded@KeisukeYamashita has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 4
🧹 Outside diff range and nitpick comments (8)
cli/src/rule/body_empty.rs (1)
8-8: LGTM! Well-implemented schema supportThe addition of conditional JSON schema generation is well-implemented and aligns perfectly with the PR's objective of improving IDE experience. The use of
cfg_attrensures the schema generation only occurs when needed, maintaining efficient compilation when the feature isn't enabled.This is a good example of using feature flags for optional functionality. The schema generation will help provide better IDE support for configuration files while keeping the core functionality lean.
cli/src/rule/scope_empty.rs (1)
7-7: Fix incorrect documentation comment.The struct documentation incorrectly mentions "subject-empty" when it should be "scope-empty" to match the actual rule implementation.
-/// ScopeEmpty represents the subject-empty rule. +/// ScopeEmpty represents the scope-empty rule.cli/src/rule/type_max_length.rs (1)
Line range hint
9-17: Consider enhancing field documentation for better schema clarity.The generated schema would benefit from more detailed field documentation:
- Add examples of valid values for
length- Document the default behavior when
levelis NoneApply this diff to improve the documentation:
pub struct TypeMaxLength { /// Level represents the level of the rule. /// - // Note that currently the default literal is not supported. - // See: https://github.com/serde-rs/serde/issues/368 + /// When not specified, defaults to Level::Error. + /// + /// Note: Default literal not supported in serde. + /// See: https://github.com/serde-rs/serde/issues/368 level: Option<Level>, /// Length represents the maximum length of the type. + /// Defaults to 72 characters. + /// Example: For length: 10, "feat" is valid but "refactoring" exceeds the limit. length: usize, }cli/src/rule/type_empty.rs (1)
Line range hint
10-14: Consider simplifying the Level field architectureThe
levelfield is wrapped inOption<Level>but effectively always has a value throughDefault. Given that:
- There's a constant
LEVEL- The field always defaults to
Some(Self::LEVEL)- The comment mentions Serde limitations
Consider either:
- Documenting why
Optionis necessary despite always having a value- Refactoring to use
Leveldirectly with#[serde(default)]This would make the schema more accurate by showing that the field always has a value.
cli/src/rule/scope_max_length.rs (3)
6-6: Fix incorrect and duplicate documentation commentsThere are two issues in the documentation:
- The first comment incorrectly mentions "description-max-length" instead of "scope-max-length"
- There's a duplicate documentation comment for ScopeMaxLength
Apply this fix:
-/// ScopeMaxLength represents the description-max-length rule. +/// ScopeMaxLength represents the scope-max-length rule. #[derive(Clone, Debug, Deserialize, Serialize)] #[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] pub struct ScopeMaxLength { -/// ScopeMaxLength represents the scope-max-length rule. impl Rule for ScopeMaxLength {Also applies to: 19-19
Line range hint
31-41: Fix incorrect handling of missing scopeThe current implementation incorrectly returns a violation when the scope is
None. This means commits without a scope will fail validation, which doesn't align with the rule's purpose of checking scope length. The rule should only validate the length when a scope is present.Apply this fix:
fn validate(&self, message: &Message) -> Option<Violation> { match &message.scope { Some(scope) => { if scope.len() >= self.length { return Some(Violation { level: self.level.unwrap_or(Self::LEVEL), message: self.message(message), }); } } - None => { - return Some(Violation { - level: self.level.unwrap_or(Self::LEVEL), - message: self.message(message), - }) - } + None => return None, // No violation for missing scope } None
Line range hint
65-108: Enhance test coverage and improve test descriptionsThe current tests could be improved with:
- Additional test cases for missing scope
- More descriptive test names
- Edge case testing around the length boundary
Consider adding these test cases:
#[test] fn test_validate_should_pass_when_scope_is_none() { let rule = ScopeMaxLength::default(); let message = Message { scope: None, ..Default::default() }; assert!(rule.validate(&message).is_none()); } #[test] fn test_validate_should_pass_when_scope_length_equals_limit() { let rule = ScopeMaxLength { length: 5, ..Default::default() }; let message = Message { scope: Some("12345".to_string()), ..Default::default() }; assert!(rule.validate(&message).is_none()); }cli/src/rule/subject_empty.rs (1)
6-6: Fix documentation inconsistency.The struct documentation comment states "SubjectEmpty represents the subject-empty rule" twice - once above the struct and once in the impl block.
Consider removing the redundant comment from one location or making them more specific:
- /// SubjectEmpty represents the subject-empty rule. + /// Rule that ensures the commit message has a non-empty subject.Also applies to: 9-15
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
cli/Cargo.toml(2 hunks)cli/json-schema/config.json(1 hunks)cli/src/config.rs(2 hunks)cli/src/git.rs(2 hunks)cli/src/rule.rs(2 hunks)cli/src/rule/body_empty.rs(1 hunks)cli/src/rule/body_max_length.rs(1 hunks)cli/src/rule/description_empty.rs(1 hunks)cli/src/rule/description_format.rs(1 hunks)cli/src/rule/description_max_length.rs(1 hunks)cli/src/rule/footers_empty.rs(1 hunks)cli/src/rule/scope.rs(1 hunks)cli/src/rule/scope_empty.rs(3 hunks)cli/src/rule/scope_format.rs(1 hunks)cli/src/rule/scope_max_length.rs(1 hunks)cli/src/rule/subject_empty.rs(2 hunks)cli/src/rule/type.rs(1 hunks)cli/src/rule/type_empty.rs(1 hunks)cli/src/rule/type_format.rs(1 hunks)cli/src/rule/type_max_length.rs(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- cli/src/git.rs
- cli/src/rule/description_max_length.rs
- cli/src/rule/footers_empty.rs
- cli/src/rule/type.rs
- cli/src/rule/scope_format.rs
- cli/src/rule/type_format.rs
- cli/src/rule/body_max_length.rs
- cli/src/rule/scope.rs
- cli/src/rule/description_format.rs
🔇 Additional comments (12)
cli/src/rule/scope_empty.rs (2)
8-8: LGTM! Clean implementation of schema support.
The conditional compilation attribute for JsonSchema derivation is correctly implemented and aligns well with the PR's objective of enhancing IDE experience.
83-83: LGTM! Clean test assertion formatting.
The test assertion formatting changes improve readability while maintaining the same functionality.
Also applies to: 102-102
cli/src/rule/type_max_length.rs (1)
8-8: LGTM! Schema generation attribute correctly added.
The conditional attribute for JSON schema generation is properly implemented and aligns with the PR's objective of improving IDE experience.
Let's verify consistent schema generation across related rule files:
✅ Verification successful
Let me check the schema attributes for all these length-related structs to ensure consistency.
Schema generation attribute is consistently implemented across all length rules
All related length rule structs (TypeMaxLength, ScopeMaxLength, BodyMaxLength, and DescriptionMaxLength) have the same conditional schema attribute properly implemented, ensuring consistent JSON schema generation when the "schemars" feature is enabled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all rule structs have consistent schema generation attributes
# Search for all rule structs and verify they have the schema attribute
rg -U "struct \w+Length.*?\{" -A 1 "cli/src/rule" | grep -E "(struct|schema)"
Length of output: 329
Script:
#!/bin/bash
# Check schema attributes for all length-related rule structs
for file in body_max_length.rs description_max_length.rs scope_max_length.rs type_max_length.rs; do
echo "=== $file ==="
rg -B 2 "struct \w+Length" "cli/src/rule/$file"
done
Length of output: 1475
cli/src/rule/type_empty.rs (1)
8-8: LGTM! The schema generation implementation looks correct.
The conditional attribute for JSON schema generation is properly implemented:
- Uses correct feature flag syntax
- Maintains backward compatibility
- Aligns with the PR's goal of enhancing IDE experience
Let's verify the consistency of schema generation across other rule types:
✅ Verification successful
Schema generation is consistently implemented across all rule types
The verification confirms that all rule structs in the codebase consistently implement the conditional JsonSchema derivation using the same pattern:
#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]This includes all rule types:
- TypeEmpty
- TypeFormat
- TypeMaxLength
- SubjectEmpty
- ScopeMaxLength
- ScopeFormat
- ScopeEmpty
- DescriptionMaxLength
- DescriptionFormat
- DescriptionEmpty
- BodyMaxLength
- BodyEmpty
- FootersEmpty
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all rule types consistently implement JsonSchema when the feature is enabled
# Expected: All rule types should have the same conditional schema derivation
# Search for all rule type structs and their schema implementation
ast-grep --pattern 'struct $name {
$$$
}' cli/src/rule/*.rs | grep -B 2 'struct' | grep -E '(JsonSchema|struct)'
Length of output: 543
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find JsonSchema usage and struct definitions
rg -B 2 "struct" cli/src/rule/ --type rust
Length of output: 10999
cli/src/rule/scope_max_length.rs (1)
8-8: LGTM: Schema generation attribute correctly implemented
The conditional schema generation attribute is properly implemented and aligns with the PR's objective of enhancing IDE experience.
cli/src/rule/subject_empty.rs (1)
8-8: LGTM! Proper conditional schema generation setup.
The conditional derive of JsonSchema is correctly implemented using cfg_attr.
cli/src/rule/description_empty.rs (2)
8-8: LGTM! Consistent schema generation implementation.
The conditional derive of JsonSchema matches the pattern used in other rule files.
8-8: Verify consistent schema implementation across all rules.
Let's ensure all rule files have the schema generation attribute consistently implemented.
✅ Verification successful
Schema generation attribute is consistently implemented across all rule files
The verification confirms that all Rust files in the cli/src/rule/ directory have the cfg_attr attribute for schema generation properly implemented. No files are missing the schema implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent schema implementation across rule files
echo "Files with JsonSchema derive:"
rg -l "cfg_attr.*schemars.*JsonSchema" cli/src/rule/
echo "\nFiles potentially missing JsonSchema derive:"
fd -e rs . cli/src/rule/ -x sh -c "if ! grep -q 'cfg_attr.*schemars.*JsonSchema' {}; then echo {}; fi"
Length of output: 784
cli/src/config.rs (1)
22-22: LGTM: Schema generation capability added
The conditional compilation attribute for JsonSchema derivation is correctly implemented.
cli/src/rule.rs (2)
31-31: LGTM: Documentation URL updated
The documentation URL has been correctly updated to the new path.
33-33: LGTM: Schema generation capability added
The conditional compilation attributes for JsonSchema derivation are correctly implemented for both the Rules struct and Level enum.
Also applies to: 231-231
cli/json-schema/config.json (1)
1-523: LGTM: Well-structured schema
The JSON schema is well-organized and comprehensive, covering all commitlint rules with proper validation constraints.
Let's verify the schema's validity:
✅ Verification successful
Let me verify the schema references with a corrected script.
Schema structure and references are valid
The verification confirms that:
- The JSON schema syntax is valid
- The root
Rulesdefinition is properly referenced inproperties.rules - All rule definitions (BodyEmpty, BodyMaxLength, etc.) are correctly referenced in the Rules properties
- The
Leveldefinition is properly referenced in all rule definitions that use it
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Validate the JSON schema syntax and structure
# Test: Validate JSON schema syntax
jq empty cli/json-schema/config.json
# Test: Verify all rule definitions are referenced
jq -r '.definitions | keys[]' cli/json-schema/config.json | while read def; do
if ! jq -e --arg def "$def" '..|.["$ref"]?|select(.=="#/definitions/"+$def)' cli/json-schema/config.json >/dev/null; then
echo "Warning: Definition '$def' is not referenced in the schema"
fi
done
Length of output: 8518
Script:
#!/bin/bash
# Description: Validate the JSON schema syntax and structure
# Test: Validate JSON schema syntax
jq empty cli/json-schema/config.json
# Test: Check references in properties.rules
jq -r '.properties.rules.allOf[].["$ref"]' cli/json-schema/config.json
# Test: Check references in rules properties
jq -r '.definitions.Rules.properties | to_entries[] | .value.anyOf[].["$ref"]' cli/json-schema/config.json
# Test: Check references in level properties
jq -r '.. | objects | select(has("level")) | .level.anyOf[].["$ref"] | select(. != null)' cli/json-schema/config.json
Length of output: 1116
Script:
#!/bin/bash
# Description: Validate the JSON schema syntax and structure with corrected jq syntax
# Test: Validate JSON schema syntax
jq empty cli/json-schema/config.json
# Test: Check references in properties.rules
jq -r '.properties.rules.allOf[0]."$ref"' cli/json-schema/config.json
# Test: Check references in rules properties
jq -r '.definitions.Rules.properties | to_entries[] | select(.value.anyOf != null) | .value.anyOf[0]."$ref"' cli/json-schema/config.json
# Test: Check references in level properties
jq -r '.. | select(type == "object") | select(.level != null) | .level.anyOf[0]."$ref"' cli/json-schema/config.json
Length of output: 1087
| @@ -17,7 +17,7 @@ edition.workspace = true | |||
| clap = { version = "4.5.4", features = ["derive", "env", "string"] } | |||
| futures = "0.3.30" | |||
| regex = "1.10.5" | |||
| schemars = "0.8.21" | |||
| schemars = { version = "0.8.21", optional = true } | |||
There was a problem hiding this comment.
💡 Codebase verification
Documentation needed for the schemars feature flag
The optional dependency is well-implemented, but it lacks documentation. Please add documentation in the README.md or cli/README.md explaining:
- The purpose of the
schemarsfeature flag - How to enable it
- When users might want to use it
🔗 Analysis chain
LGTM! Well-structured feature flag implementation.
The optional dependency and feature flag setup looks good. Making schemars optional and not enabled by default is the right approach since schema generation is a development-time feature.
Let's verify that the feature flag is properly documented:
Also applies to: 33-35
Let me check if there's any documentation about this feature in other common documentation locations before making the final response.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the feature is documented in README or other docs
rg -i "schemars.*feature" README.md || echo "Feature documentation not found in README.md"
Length of output: 136
Script:
#!/bin/bash
# Check docs directory and other common documentation locations
fd -e md -e txt . | xargs rg -i "schemars.*feature"
# Also check if there's any documentation in the cli directory itself
fd -e md -e txt . cli/ | xargs rg -i "schemars.*feature"
# Look for any general feature documentation that might include schemars
fd -e md -e txt . | xargs rg -i "feature.*schema"
Length of output: 164
| @@ -5,6 +5,7 @@ use super::Level; | |||
|
|
|||
| /// DescriptionEmpty represents the subject-empty rule. | |||
There was a problem hiding this comment.
Fix incorrect documentation.
The struct documentation incorrectly states "DescriptionEmpty represents the subject-empty rule" when it should be "description-empty rule".
Apply this fix:
- /// DescriptionEmpty represents the subject-empty rule.
+ /// DescriptionEmpty represents the description-empty rule.📝 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.
| /// DescriptionEmpty represents the subject-empty rule. | |
| /// DescriptionEmpty represents the description-empty rule. |
cli/src/config.rs
Outdated
| // TODO assert insta snapshot | ||
| fn generate_json_schema() { | ||
| use std::fs; | ||
|
|
||
| use crate::config::Config; | ||
| let config_schema = schemars::schema_for!(Config); | ||
| let config_schema_json = serde_json::to_string_pretty(&config_schema).unwrap(); | ||
| fs::write("json-schema/config.json", config_schema_json).unwrap(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test robustness and maintainability
The schema generation test has several areas for improvement:
- The hardcoded path could cause issues in different environments
- Missing assertions to verify the generated schema
- No cleanup of generated files after test
Consider this improved implementation:
#[cfg(feature = "schemars")]
#[test]
-// TODO assert insta snapshot
fn generate_json_schema() {
use std::fs;
+ use std::path::Path;
use crate::config::Config;
+
+ let output_dir = Path::new("json-schema");
+ fs::create_dir_all(output_dir).unwrap();
+ let output_path = output_dir.join("config.json");
+
let config_schema = schemars::schema_for!(Config);
let config_schema_json = serde_json::to_string_pretty(&config_schema).unwrap();
- fs::write("json-schema/config.json", config_schema_json).unwrap();
+ fs::write(&output_path, &config_schema_json).unwrap();
+
+ // Verify the schema is valid JSON and contains expected structure
+ let schema: serde_json::Value = serde_json::from_str(&config_schema_json).unwrap();
+ assert!(schema.get("definitions").is_some());
+ assert!(schema.get("properties").is_some());
+
+ // Cleanup
+ fs::remove_file(&output_path).unwrap();
}📝 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.
| // TODO assert insta snapshot | |
| fn generate_json_schema() { | |
| use std::fs; | |
| use crate::config::Config; | |
| let config_schema = schemars::schema_for!(Config); | |
| let config_schema_json = serde_json::to_string_pretty(&config_schema).unwrap(); | |
| fs::write("json-schema/config.json", config_schema_json).unwrap(); | |
| } | |
| #[cfg(feature = "schemars")] | |
| #[test] | |
| fn generate_json_schema() { | |
| use std::fs; | |
| use std::path::Path; | |
| use crate::config::Config; | |
| let output_dir = Path::new("json-schema"); | |
| fs::create_dir_all(output_dir).unwrap(); | |
| let output_path = output_dir.join("config.json"); | |
| let config_schema = schemars::schema_for!(Config); | |
| let config_schema_json = serde_json::to_string_pretty(&config_schema).unwrap(); | |
| fs::write(&output_path, &config_schema_json).unwrap(); | |
| // Verify the schema is valid JSON and contains expected structure | |
| let schema: serde_json::Value = serde_json::from_str(&config_schema_json).unwrap(); | |
| assert!(schema.get("definitions").is_some()); | |
| assert!(schema.get("properties").is_some()); | |
| // Cleanup | |
| fs::remove_file(&output_path).unwrap(); | |
| } |
cli/json-schema/config.json
Outdated
| "DescriptionEmpty": { | ||
| "description": "DescriptionEmpty represents the subject-empty rule.", | ||
| "type": "object", |
There was a problem hiding this comment.
Fix incorrect rule descriptions
Several rule descriptions are incorrect:
- DescriptionEmpty is described as "subject-empty"
- Scope is described as "subject-empty"
- Type is described as "subject-empty"
Apply these corrections:
"DescriptionEmpty": {
- "description": "DescriptionEmpty represents the subject-empty rule.",
+ "description": "DescriptionEmpty represents the description-empty rule.",
"type": "object",
"Scope": {
- "description": "Scope represents the subject-empty rule.",
+ "description": "Scope represents the scope rule.",
"type": "object",
"Type": {
- "description": "Type represents the subject-empty rule.",
+ "description": "Type represents the type rule.",
"type": "object",Also applies to: 313-314, 429-430
There was a problem hiding this comment.
I created another branch that could fix this inconsistency by using make_rule! macros to create each rule struct, with predefined docs (which can only be output to the json schema using schemars@v1-alpha.x if you are interested i can edit the PR to point to this branch @KeisukeYamashita.
There was a problem hiding this comment.
@elcoosp, thanks for letting me know about the new branch and your plan to fix the inconsistency using the make_rule! macros and schemars@v1-alpha.x. If you need any assistance or have any questions, feel free to ask.
✏️ Learnings added
Learnt from: elcoosp
PR: KeisukeYamashita/commitlint-rs#389
File: cli/json-schema/config.json:63-65
Timestamp: 2024-12-02T11:26:43.014Z
Learning: To address inconsistencies in rule descriptions within the JSON schema, `make_rule!` macros and `schemars@v1-alpha.x` can be used to create rule structs with predefined documentation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Hello, in my humble opinion, I think it is out of this PR scope!
Could you file another PR, please? Thank you in advance!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
Hello, thank you for raising this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cli/src/config.rs (1)
130-140: Consider build-time schema generationWhile generating the schema during tests works, it might not be the optimal approach for improving IDE experience. Consider:
- Moving schema generation to build time using a build script (
build.rs)- This ensures the schema is always available with the package
- IDEs can find and use the schema without running tests
Would you like me to provide an example implementation using a build script?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
cli/src/config.rs(2 hunks)
🔇 Additional comments (2)
cli/src/config.rs (2)
22-22: LGTM: Clean feature-gated schema support
The conditional derive macro is well-implemented, maintaining backward compatibility while enabling JSON schema generation when the feature is enabled.
132-140: Improve test robustness and maintainability
The test implementation has several areas that need improvement, including hardcoded paths, missing validations, and cleanup. These issues were previously identified in an earlier review.
KeisukeYamashita
changed the title
Signed-off-by: KeisukeYamashita <19yamashita15@gmail.com>
Signed-off-by: KeisukeYamashita <19yamashita15@gmail.com>
KeisukeYamashita
changed the title
|
Thank you for your fantastic suggestion! I'd like to not generate the schema on test because we either have to generate and then commit it or commit the generated schema on CI. Instead, I updated it so that the schema is added to each release. |
Signed-off-by: KeisukeYamashita <19yamashita15@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
cli/Cargo.toml (1)
2-2: Consider aligning binary name with package nameThe package is named "cli" but the binary is named "commitlint". This could be confusing for users and maintainers. Consider either:
- Keeping the original package name "commitlint-rs" to match the binary name
- Renaming the binary to "cli" to match the package name
Also applies to: 16-18
schema/src/main.rs (2)
14-20: Add progress and success feedbackThe CLI should provide feedback about the schema generation process.
fn main() { let args = Args::parse(); + println!("Generating JSON schema..."); let config_schema = schemars::schema_for!(Config); let config_schema_json = serde_json::to_string_pretty(&config_schema).unwrap(); fs::write(&args.path, config_schema_json).unwrap(); + println!("Schema successfully written to: {}", args.path); }
5-12: Enhance CLI documentationThe command-line help could be more descriptive about the schema's purpose and format.
/// CLI Arguments #[derive(Parser, Debug)] -#[command(author, version, about, long_about = None)] +#[command( + author, + version, + about = "Generates JSON Schema for commitlint configuration", + long_about = "Generates a JSON Schema file that can be used to validate commitlint configuration files and provide IDE support through schema validation." +)] struct Args { /// Path to save the JSON schema + /// The generated schema will be in JSON format and can be referenced in your IDE #[arg(long, short)] path: String, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.github/workflows/release.yaml(2 hunks)Cargo.toml(1 hunks)cli/Cargo.toml(2 hunks)cli/src/lib.rs(1 hunks)cli/src/message.rs(0 hunks)schema/Cargo.toml(1 hunks)schema/src/main.rs(1 hunks)
💤 Files with no reviewable changes (1)
- cli/src/message.rs
✅ Files skipped from review due to trivial changes (2)
- cli/src/lib.rs
- schema/Cargo.toml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yaml
146-146: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (4)
Cargo.toml (1)
2-2: LGTM! Workspace configuration is properly updated.
The addition of the "schema" package to workspace members is well-structured and aligns with the PR objectives.
cli/Cargo.toml (1)
24-24: Documentation needed for the schemars feature flag
The optional dependency and feature flag setup needs documentation explaining its purpose and usage.
Also applies to: 30-32
.github/workflows/release.yaml (2)
129-129: LGTM: Job dependencies are properly configured
The addition of 'schema' and 'web' as dependencies for the publish job ensures correct execution order.
142-156: LGTM: Schema generation job is well structured
The schema job implementation follows best practices:
- Uses minimal profile for faster builds
- Includes proper artifact handling with error checking
- Clear command structure for schema generation
🧰 Tools
🪛 actionlint (1.7.4)
146-146: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| let config_schema_json = serde_json::to_string_pretty(&config_schema).unwrap(); | ||
| fs::write(&args.path, config_schema_json).unwrap(); |
There was a problem hiding this comment.
Replace unwrap() with proper error handling
The current implementation uses unwrap() which could cause panics. Consider implementing proper error handling:
- let config_schema_json = serde_json::to_string_pretty(&config_schema).unwrap();
- fs::write(&args.path, config_schema_json).unwrap();
+ let config_schema_json = serde_json::to_string_pretty(&config_schema)
+ .map_err(|e| format!("Failed to serialize schema: {}", e))?;
+ fs::write(&args.path, config_schema_json)
+ .map_err(|e| format!("Failed to write schema to {}: {}", args.path, e))?;📝 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.
| let config_schema_json = serde_json::to_string_pretty(&config_schema).unwrap(); | |
| fs::write(&args.path, config_schema_json).unwrap(); | |
| let config_schema_json = serde_json::to_string_pretty(&config_schema) | |
| .map_err(|e| format!("Failed to serialize schema: {}", e))?; | |
| fs::write(&args.path, config_schema_json) | |
| .map_err(|e| format!("Failed to write schema to {}: {}", args.path, e))?; |
| /// Path to save the JSON schema | ||
| #[arg(long, short)] | ||
| path: String, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add path validation and create parent directories
The path argument should be validated and parent directories should be created if they don't exist.
struct Args {
/// Path to save the JSON schema
#[arg(long, short)]
path: String,
}
+fn ensure_parent_dirs(path: &str) -> std::io::Result<()> {
+ if let Some(parent) = std::path::Path::new(path).parent() {
+ std::fs::create_dir_all(parent)?;
+ }
+ Ok(())
+}Update the main function to use this:
let args = Args::parse();
+ ensure_parent_dirs(&args.path)?;Committable suggestion skipped: line range outside the PR's diff.
.github/workflows/release.yaml
Outdated
| - uses: actions-rs/toolchain@v1 | ||
| with: | ||
| profile: minimal | ||
| toolchain: stable | ||
| override: true |
There was a problem hiding this comment.
Update outdated GitHub Action
The actions-rs/toolchain@v1 action is outdated. Consider updating to a newer alternative like dtolnay/rust-toolchain@stable.
Apply this change:
- - uses: actions-rs/toolchain@v1
- with:
- profile: minimal
- toolchain: stable
- override: true
+ - uses: dtolnay/rust-toolchain@stable📝 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.
| - uses: actions-rs/toolchain@v1 | |
| with: | |
| profile: minimal | |
| toolchain: stable | |
| override: true | |
| - uses: dtolnay/rust-toolchain@stable |
🧰 Tools
🪛 actionlint (1.7.4)
146-146: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/release.yaml
Outdated
| @@ -135,9 +136,25 @@ | |||
| merge-multiple: true | |||
| - uses: ncipollo/release-action@v1 | |||
| with: | |||
| artifacts: commitlint/commitlint-*.tar.gz | |||
| artifacts: commitlint/commitlint-*.tar.gz,schema.json | |||
There was a problem hiding this comment.
💡 Codebase verification
Path mismatch detected in schema.json artifact handling
The verification confirms there is a mismatch between:
- Upload path:
schema/schema.json - Artifact reference:
schema.json
The schema job uploads the artifact to schema/schema.json, but the publish job tries to access it at schema.json, which will fail. The artifact path in the publish job should be updated to match the upload path.
🔗 Analysis chain
Verify schema.json artifact path
The artifact path 'schema.json' might not match the upload path 'schema/schema.json' from the schema job.
Let's verify the artifact paths:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the schema.json path in the publish job matches the upload path
if ! rg -l "path: schema/schema.json" && rg -l "artifacts:.*schema\.json"; then
echo "Warning: Potential path mismatch between schema job upload and publish job artifact paths"
fi
Length of output: 69
Script:
#!/bin/bash
# Let's search for schema-related artifact paths more broadly
echo "=== Searching for schema artifact uploads ==="
rg -A 2 "upload.*artifact" | grep -i "schema"
echo -e "\n=== Searching for schema paths in workflow ==="
rg -A 2 "path:.*schema" .github/workflows/
echo -e "\n=== Checking artifact references ==="
rg "artifacts:.*schema" .github/workflows/
Length of output: 693
KeisukeYamashita
merged commit 91bd0a7
into
KeisukeYamashita:main
Why
For better IDE experience.
Summary by CodeRabbit
New Features
Bug Fixes
validatefunction in the message module for improved performance.Documentation
Chores
Cargo.tomlfor the schema package with necessary dependencies.