Add Direct CLI Commands for Adding FDML Entities with Comprehensive Documentation - #11
Conversation
Co-authored-by: kolanski <632928+kolanski@users.noreply.github.com>
Co-authored-by: kolanski <632928+kolanski@users.noreply.github.com>
|
@copilot don't forget add this with usage examples to README and in help cli function too! |
|
@copilot status? |
There was a problem hiding this comment.
Pull Request Overview
This PR implements direct CLI commands for adding FDML entities to specification files without requiring manual migration file creation. The implementation leverages the existing migration system internally while providing a simpler user interface for common operations.
Key Changes
- Extended migration system with new operations for adding/removing entities, actions, and constraints
- Added new CLI commands for direct entity management and listing operations
- Implemented smart file discovery and temporary migration handling for seamless user experience
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/cli/args.rs | Added new CLI command structures for Add and List operations with comprehensive argument definitions |
| src/cli/commands.rs | Implemented command execution logic with temporary migration handling and file discovery |
| src/cli/mod.rs | Updated module exports to include new command types |
| src/migration/runner.rs | Extended migration operations and added execution logic for new entity types |
| src/migration/tests.rs | Added comprehensive unit tests for new migration operations |
| tests/cli_tests.rs | Added integration tests covering all new CLI commands and error scenarios |
| use crate::error::Result; | ||
| use crate::parser::{parse_fdml_yaml}; | ||
| use crate::parser::ast::{FdmlDocument, Feature, Scenario, Field, Value}; | ||
| use crate::parser::ast::{FdmlDocument, Feature, Scenario, Field, Value, Entity, Action, Constraint}; |
There was a problem hiding this comment.
[nitpick] The import list is becoming lengthy. Consider using a wildcard import or grouping related imports to improve readability: use crate::parser::ast::*; or separate the core types from the new types.
| use crate::parser::ast::{FdmlDocument, Feature, Scenario, Field, Value, Entity, Action, Constraint}; | |
| use crate::parser::ast::*; |
| // Try to parse as different types | ||
| if let Ok(b) = d.parse::<bool>() { | ||
| serde_json::Value::Bool(b) | ||
| } else if let Ok(n) = d.parse::<f64>() { | ||
| serde_json::Value::Number(serde_json::Number::from_f64(n).unwrap()) | ||
| } else { | ||
| serde_json::Value::String(d) | ||
| } | ||
| }); |
There was a problem hiding this comment.
The unwrap() call on line 447 can panic if the float is NaN or infinite. Use serde_json::Number::from_f64(n).ok_or_else(|| error) to handle this case properly.
| } | ||
|
|
||
| // Clean up temporary directory | ||
| std::fs::remove_dir_all(&temp_dir).ok(); |
There was a problem hiding this comment.
[nitpick] Silently ignoring cleanup errors could mask filesystem issues. Consider logging the error or using a more explicit approach: if let Err(e) = std::fs::remove_dir_all(&temp_dir) { eprintln!(\"Warning: Failed to cleanup temporary directory: {}\", e); }
| std::fs::remove_dir_all(&temp_dir).ok(); | |
| if let Err(e) = std::fs::remove_dir_all(&temp_dir) { | |
| print_warning(&format!("Warning: Failed to cleanup temporary directory '{}': {}", temp_dir.display(), e)); | |
| } |
| fs::write(&test_file, r#" | ||
| metadata: | ||
| version: "1.3" | ||
| author: "Test" | ||
| description: "Test file" | ||
|
|
||
| entities: [] | ||
| actions: [] | ||
| features: [] | ||
| constraints: [] | ||
| "#).unwrap(); |
There was a problem hiding this comment.
[nitpick] The test FDML content is duplicated across multiple test functions. Consider extracting this into a helper function create_minimal_fdml_file(path: &Path) to reduce code duplication and improve maintainability.
| MigrationOperation::AddConstraint { id, name, condition, applies_to, .. } => { | ||
| if id.trim().is_empty() || name.trim().is_empty() || condition.trim().is_empty() || applies_to.trim().is_empty() { | ||
| return Err(crate::error::FdmlError::migration_error( | ||
| "AddConstraint operation requires non-empty id, name, condition, and applies_to".to_string() | ||
| )); | ||
| } | ||
| }, |
There was a problem hiding this comment.
[nitpick] The validation logic is repetitive across different operation types. Consider extracting a helper function validate_non_empty_fields(fields: &[(&str, &str)]) -> Result<()> to reduce code duplication.
Added comprehensive usage examples to both README.md and CLI help functions! README Updates:
CLI Help Enhancements:
The CLI now provides rich help at every level: fdml --help # Main help with examples
fdml add --help # Add commands with examples
fdml add entity --help # Specific command help
fdml list --help # List commands with examplesAll implemented in commit 8ac703e.
Status: Complete! ✅ All requested direct CLI commands have been implemented with comprehensive documentation: ✅ Completed:
📚 Documentation:
The implementation is production-ready with full backward compatibility. Users can now easily add entities, features, actions, and constraints directly via CLI without manual migration files. |
Implements direct CLI commands for adding FDML entities without requiring manual migration file creation, with comprehensive usage examples and enhanced help documentation.
Overview
This PR adds complete support for direct CLI operations that leverage the existing migration system internally to add FDML entities directly to specification files. Users can now perform common operations without the complexity of creating migration files manually, with rich documentation and user-friendly help text.
New Commands Added
Core Add Commands
fdml add feature <id> --title "Feature Title" [--description "desc"] [--target file.fdml]fdml add entity <id> --name "Entity Name" [--description "desc"] [--target file.fdml]fdml add action <id> --name "Action Name" [--description "desc"] [--target file.fdml]fdml add constraint <id> --name "Name" --condition "condition" --applies-to "target" [--target file.fdml]Field Management
fdml add field <entity_id> <field_name> --field-type <type> [--required] [--default value] [--target file.fdml]List Commands
fdml list features [--target file.fdml]fdml list entities [--target file.fdml]fdml list actions [--target file.fdml]fdml list constraints [--target file.fdml]Example Usage
Documentation & Help Enhancements
README.md Updates
Enhanced CLI Help
fdml --help,fdml add --help,fdml add entity --helpImplementation Details
Extended Migration System
AddEntity,RemoveEntity,AddAction,RemoveAction,AddConstraint,RemoveConstraintexecute_operation,describe_operation, andvalidate_operationmethods to handle new operationsSmart Target File Handling
--target file.fdmlparameter for all operationsspec.fdml,main.fdml, etc.)Internal Architecture
Enhanced User Experience
Testing
Added comprehensive test coverage including:
Breaking Changes
None. All existing CLI commands and migration functionality remain unchanged.
Files Modified
README.md: Added comprehensive usage examples and command documentationsrc/cli/args.rs: Enhanced CLI help text with detailed examples and descriptionssrc/migration/runner.rs: Added new migration operations and execution logicsrc/cli/commands.rs: Implemented command execution logicsrc/migration/tests.rs: Added unit tests for new migration operationstests/cli_tests.rs: Added integration tests for new CLI commandsThis implementation provides a user-friendly interface for common FDML operations while maintaining the robustness and safety of the underlying migration system, now with comprehensive documentation that makes the tools accessible to all users.
This pull request was created as a result of the following prompt from Copilot chat.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.