Skip to content

Commit 669e580

Browse files
michielmichiel
authored
Merge pull request #8 from michiel/feature/model-test
Refactoring and bug fixes
2 parents a5a96ea + d4ba00e commit 669e580

File tree

9 files changed

+976
-458
lines changed

9 files changed

+976
-458
lines changed

‎CLAUDE.md

+25
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
# CLAUDE.md - Layercake Tool Development Guide
2+
3+
## Build/Test Commands
4+
- Build: `cargo build`
5+
- Run: `cargo run -- -p sample/ref/plan.yaml`
6+
- Watch mode: `cargo run -- -p sample/ref/plan.yaml -w`
7+
- Run tests: `cargo test`
8+
- Run single test: `cargo test reference_exports`
9+
- With log levels: `cargo run -- --log-level debug -p sample/ref/plan.yaml`
10+
11+
## Code Style Guidelines
12+
- **Imports**: Group standard library, external crates, then internal modules
13+
- **Formatting**: Use Rust standard formatting (rustfmt)
14+
- **Error Handling**: Use `anyhow::Result` for most operations; propagate errors with `?`
15+
- **Naming**:
16+
- Use snake_case for variables, functions, modules
17+
- Use CamelCase for types and enums
18+
- **File Organization**: Keep related functionality in the same module
19+
- **Documentation**: Document public APIs with /// comments
20+
- **Type Safety**: Use strong typing and avoid unwraps when possible
21+
22+
## Project Structure
23+
- CSV files define nodes, edges, and layers
24+
- Plan files (YAML) define transformation and export steps
25+
- Custom renderers use Handlebars templates

‎REFACTORING.md

+71
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
# Refactoring Improvements
2+
3+
This document outlines the refactoring changes that were made to improve the codebase maintainability.
4+
5+
## 1. Improved Export Module Organization
6+
7+
Added a common renderer module in `export/mod.rs` to reduce duplication across export implementations:
8+
9+
- Created `renderer::render_template` and `renderer::create_standard_context` functions
10+
- Updated exporters (dot, plantuml) to use the common renderer
11+
- This reduces code duplication and makes it easier to add new exporters
12+
13+
## 2. Refactored Plan Execution
14+
15+
Broke down the large `run_plan` function in `plan_execution.rs` into smaller, more focused functions:
16+
17+
- `create_graph_from_plan` - Initializes a new graph with metadata
18+
- `load_data_into_graph` - Loads data from import profiles
19+
- `apply_graph_transformations` - Applies various transforms to the graph
20+
- `export_graph` - Handles exporting to different formats
21+
- `watch_for_changes` - Set up file watching
22+
23+
This improves readability, makes the code easier to test, and separates concerns.
24+
25+
## 3. Added Documentation
26+
27+
Added function documentation comments to explain:
28+
29+
- Purpose of functions
30+
- Parameter meanings
31+
- Return values
32+
- Error conditions
33+
34+
## 4. Improved Error Handling
35+
36+
- Made error messages more descriptive
37+
- Used proper error handling patterns with `anyhow::Error`
38+
- Added explicit error type conversions rather than unsafe propagation
39+
40+
## 5. Organized Helper Functions
41+
42+
- Clarified the scope and use of common helper functions
43+
- Added private helper methods to the `Graph` implementation
44+
- Ensured consistent naming patterns for helpers
45+
46+
## 6. Added Tests
47+
48+
Added tests for previously untested complex methods:
49+
50+
- `test_invert_graph`
51+
- `test_modify_graph_limit_partition_width`
52+
- `test_modify_graph_limit_partition_depth`
53+
- `test_aggregate_edges`
54+
- `test_verify_graph_integrity`
55+
56+
## 7. Fixed Bugs
57+
58+
- Fixed a bug in `verify_graph_integrity` where non-partition nodes were incorrectly identified
59+
- Added proper test to verify the fix
60+
61+
## 8. Addressed Deprecated Methods
62+
63+
- Marked `get_node` as deprecated in favor of `get_node_by_id` for a more consistent API
64+
- Made `get_node` an alias to `get_node_by_id` to maintain backward compatibility
65+
66+
## Remaining Work
67+
68+
- Fix the integration test that's now failing
69+
- Replace the remaining uses of deprecated `get_node` with `get_node_by_id`
70+
- Fix the remaining warnings for unused mutable variables
71+
- Add more comprehensive tests for edge cases

‎src/export/mod.rs

+36
Original file line numberDiff line numberDiff line change
@@ -9,3 +9,39 @@ pub mod to_jsgraph;
99
pub mod to_json;
1010
pub mod to_mermaid;
1111
pub mod to_plantuml;
12+
13+
/// Common rendering function used by all exporters
14+
/// This helps eliminate duplication across export modules
15+
pub mod renderer {
16+
use crate::graph::Graph;
17+
use crate::plan::RenderConfig;
18+
use serde_json::{json, Value};
19+
use std::error::Error;
20+
21+
/// Standard rendering function for template-based exports
22+
pub fn render_template(
23+
graph: Graph,
24+
render_config: RenderConfig,
25+
template: &str
26+
) -> Result<String, Box<dyn Error>> {
27+
let handlebars = crate::common::get_handlebars();
28+
29+
let context = create_standard_context(graph, render_config);
30+
31+
let res = handlebars.render_template(template, &context)?;
32+
Ok(res)
33+
}
34+
35+
/// Creates a standard context object used for most templates
36+
pub fn create_standard_context(graph: Graph, render_config: RenderConfig) -> Value {
37+
json!({
38+
"config": render_config,
39+
"hierarchy_nodes": graph.get_hierarchy_nodes(),
40+
"hierarchy_edges": graph.get_hierarchy_edges(),
41+
"hierarchy_tree": graph.build_json_tree(),
42+
"flow_nodes": graph.get_non_partition_nodes(),
43+
"flow_edges": graph.get_non_partition_edges(),
44+
"layers": graph.get_layer_map(),
45+
})
46+
}
47+
}

‎src/export/to_dot.rs

+6-59
Original file line numberDiff line numberDiff line change
@@ -2,71 +2,18 @@ use crate::graph::Graph;
22
use crate::plan::RenderConfig;
33
use std::error::Error;
44

5+
/// Renders a graph to the DOT format for use with Graphviz
56
pub fn render(graph: Graph, render_config: RenderConfig) -> Result<String, Box<dyn Error>> {
6-
use serde_json::json;
7-
8-
let tree = graph.build_json_tree();
9-
let handlebars = crate::common::get_handlebars();
10-
11-
let res = handlebars.render_template(
12-
&get_template(),
13-
&json!({
14-
"config": render_config,
15-
"hierarchy_nodes": graph.get_hierarchy_nodes(),
16-
"hierarchy_edges": graph.get_hierarchy_edges(),
17-
"hierarchy_tree": tree,
18-
"flow_nodes": graph.get_non_partition_nodes(),
19-
"flow_edges": graph.get_non_partition_edges(),
20-
"layers": graph.get_layer_map(),
21-
}),
22-
)?;
23-
Ok(res)
7+
super::renderer::render_template(graph, render_config, &get_template())
248
}
259

10+
/// Returns the Handlebars template for DOT format
2611
pub fn get_template() -> String {
2712
include_str!("to_dot.hbs").to_string()
2813
}
2914

3015
#[cfg(test)]
3116
mod tests {
32-
33-
// #[test]
34-
// fn graphviz_template_can_render() {
35-
// let handlebars = get_handlebars();
36-
// let res = handlebars
37-
// .render_template(
38-
// &get_template(),
39-
// &json!({
40-
// "nodes": [
41-
// {
42-
// "id": "id1",
43-
// "layer": "layer1",
44-
// "label": "Node 1"
45-
// },
46-
// {
47-
// "id": "id2",
48-
// "layer": "layer1",
49-
// "label": "Node 2"
50-
// },
51-
// {
52-
// "id": "id3",
53-
// "layer": "layer2",
54-
// "label": "Node 3"
55-
// },
56-
// {
57-
// "id": "id4",
58-
// "label": "Node 4"
59-
// }
60-
// ],
61-
// "edges": [
62-
// ]
63-
// }),
64-
// )
65-
// .expect("This to render");
66-
//
67-
// assert_eq!(
68-
// res,
69-
// "\n\ndigraph G {\n rankdir=\"TB\";\n splines=true;\n overlap=false;\n nodesep=\"0.3\";\n ranksep=\"1.2\";\n labelloc=\"t\";\n fontname=\"Lato\";\n node [ shape=\"plaintext\" style=\"filled, rounded\" fontname=\"Lato\" margin=0.2 ]\n edge [ fontname=\"Lato\" color=\"#2B303A\" ]\n\n\n}\n "
70-
// );
71-
// }
72-
}
17+
// These tests are commented out but could be rewritten
18+
// using the new rendering approach
19+
}

‎src/export/to_plantuml.rs

+6-115
Original file line numberDiff line numberDiff line change
@@ -2,127 +2,18 @@ use crate::graph::Graph;
22
use crate::plan::RenderConfig;
33
use std::error::Error;
44

5+
/// Renders a graph to PlantUML format
56
pub fn render(graph: Graph, render_config: RenderConfig) -> Result<String, Box<dyn Error>> {
6-
use serde_json::json;
7-
8-
let handlebars = crate::common::get_handlebars();
9-
let res = handlebars.render_template(
10-
&get_template(),
11-
&json!({
12-
"config": render_config,
13-
"hierarchy_nodes": graph.get_hierarchy_nodes(),
14-
"hierarchy_edges": graph.get_hierarchy_edges(),
15-
"hierarchy_tree": graph.build_json_tree(),
16-
"flow_nodes": graph.get_non_partition_nodes(),
17-
"flow_edges": graph.get_non_partition_edges(),
18-
"layers": graph.get_layer_map(),
19-
}),
20-
)?;
21-
Ok(res)
7+
super::renderer::render_template(graph, render_config, &get_template())
228
}
239

10+
/// Returns the Handlebars template for PlantUML format
2411
pub fn get_template() -> String {
2512
include_str!("to_plantuml.hbs").to_string()
2613
}
2714

2815
#[cfg(test)]
2916
mod tests {
30-
31-
// #[test]
32-
// fn plantuml_template_can_render() {
33-
// let handlebars = get_handlebars();
34-
// let res = handlebars
35-
// .render_template(
36-
// &get_template(),
37-
// &json!({
38-
// "nodes": [{
39-
// "id": "id1",
40-
// "label": "Root",
41-
// "layer": "Layer1",
42-
// "is_partition": true,
43-
// "belongs_to": null,
44-
// "comment": null,
45-
// },
46-
// {
47-
// "id": "id2",
48-
// "label": "Child1",
49-
// "layer": "Layer1",
50-
// "is_partition": false,
51-
// "belongs_to": "id1",
52-
// "comment": null,
53-
// },
54-
// {
55-
// "id": "3",
56-
// "label": "Child2",
57-
// "layer": "Layer1",
58-
// "is_partition": false,
59-
// "belongs_to": "id1",
60-
// "comment": null,
61-
// }
62-
// ],
63-
// "edges": [
64-
// {
65-
// "source": "id1",
66-
// "target": "id2",
67-
// "label": "belongs_to",
68-
// "layer": "nesting",
69-
// "comment": null,
70-
// },
71-
// {
72-
// "source": "id1",
73-
// "target": "id3",
74-
// "label": "belongs_to",
75-
// "layer": "nesting",
76-
// "comment": null,
77-
// }
78-
// ],
79-
// "tree" : [{
80-
// "id": "id1",
81-
// "label": "Root",
82-
// "layer": "Layer1",
83-
// "is_partition": true,
84-
// "belongs_to": null,
85-
// "comment": null,
86-
// "children": [
87-
// {
88-
// "id": "id2",
89-
// "label": "Child1",
90-
// "layer": "Layer1",
91-
// "is_partition": false,
92-
// "belongs_to": "1",
93-
// "comment": null,
94-
// "children": []
95-
// },
96-
// {
97-
// "id": "id3",
98-
// "label": "Child2",
99-
// "layer": "Layer1",
100-
// "is_partition": false,
101-
// "belongs_to": "1",
102-
// "comment": null,
103-
// "children": []
104-
// }
105-
// ]
106-
// }]
107-
// }),
108-
// )
109-
// .expect("This to render");
110-
//
111-
// assert_eq!(
112-
// res,
113-
// r##"
114-
// @startuml
115-
//
116-
// rectangle "Root" as id1 {
117-
// rectangle "Child1" as id2
118-
// rectangle "Child2" as id3
119-
// }
120-
//
121-
// id1 --> id2
122-
// id1 --> id3
123-
//
124-
// @enduml
125-
// "##
126-
// );
127-
// }
128-
}
17+
// These tests are commented out but could be rewritten
18+
// using the new rendering approach
19+
}

0 commit comments

Comments
 (0)