Refactoring of project structure #1
Conversation
…XML attributes. Value can be stored in child element as fallback.
…on for enum serialization changed in `serde-xml-rs`.
… mark its membership in the `layout` functionality.
…ey are valid numbers.
…g (easier interop with other formats).
…rializing_none]` consistently across types.
…o shared by other io format libraries.
…ll defined invariants.
…and `thiserror`.
…ducibility, and supports `Eq`).
refactor: Move common validation tests into utility methods.
feat: Add validation functionality for `BmaLayoutVariable`.
…g a bit more structured.
…e name in BMA model).
…n (still serializes to `Model` for compatibility).
…, plus updated error handling during ser/deser.
WalkthroughThis update restructures the codebase to modularize the BMA model, network, variable, relationship, and layout components into distinct files, introducing robust validation and error-reporting frameworks. JSON and XML serialization/deserialization are refactored with intermediate types and unified handling of quoted numbers. Numerous internal APIs, utility functions, and comprehensive tests are added or improved. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant JSON/XML Data
participant JsonBmaModel/XmlBmaModel
participant BmaModel
participant Validation
participant Reporter
User->>JSON/XML Data: Provide serialized model
JSON/XML Data->>JsonBmaModel/XmlBmaModel: Deserialize to intermediate struct
JsonBmaModel/XmlBmaModel->>BmaModel: Convert to internal model (TryFrom)
BmaModel->>Validation: Call validate_all
Validation->>Reporter: Collect errors
Reporter-->>User: Return validation results
Poem
✨ Finishing Touches
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. 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: 14
🔭 Outside diff range comments (1)
src/model/_impl_xml_serde.rs (1)
139-153: 🛠️ Refactor suggestionUpdate function calls to handle Result types.
With the updated function signatures returning
Result, the calling code needs to be updated to handle the potential errors.variables: xml_model .variables .variable .into_iter() - .map(Self::convert_xml_layout_var) - .collect(), + .map(Self::convert_xml_layout_var) + .collect::<Result<Vec<BmaLayoutVariable>, String>>()?, containers: xml_model .containers .unwrap_or(XmlContainers { container: Vec::new(), }) .container .into_iter() - .map(Self::convert_xml_container) - .collect(), + .map(Self::convert_xml_container) + .collect::<Result<Vec<BmaLayoutContainer>, String>>()?,
🧹 Nitpick comments (12)
.idea/dictionaries/project.xml (1)
1-7: Exclude IDE-specific config from version control
IDE settings under.idea/are typically user‐specific and can clutter the repo. Consider removing this file from source control and adding.idea/(or at least*.xmlin that folder) to.gitignoreto keep the codebase clean.src/update_fn/parser.rs (1)
121-131: Consider simplifying redundant return pattern.The code is syntactically correct, but there's a redundant
returnat the beginning of theif letexpression since both branches already return.Consider this cleaner approach:
- return if let BmaFnToken::TokenList(inner_token_list) = *argument.clone() { - Ok(BmaFnUpdate::mk_unary( - parse_bma_fn_tokens(&inner_token_list)?, - operator.clone(), - )) - } else { - return Err( - "Function must be applied on `BmaFnToken::TokenList` args.".to_string() - ); - }; + if let BmaFnToken::TokenList(inner_token_list) = *argument.clone() { + return Ok(BmaFnUpdate::mk_unary( + parse_bma_fn_tokens(&inner_token_list)?, + operator.clone(), + )); + } else { + return Err( + "Function must be applied on `BmaFnToken::TokenList` args.".to_string() + ); + }src/serde/quote_num.rs (1)
7-8: Consider adding validation for numeric range.The struct currently accepts any
u32value, but depending on the domain constraints, you might want to add validation to ensure the values are within expected ranges for BMA model IDs.src/model/mod.rs (2)
20-33: Test helper creates inconsistent variable IDs.The
simple_network()function creates variables with IDs 3 and 0, but then references them in relationships as IDs 0 and 3. This creates a potential confusion where the variable ordering in the vector doesn't match the ID usage.Consider making the variable IDs consistent with their positions or add comments to clarify the mapping:
variables: vec![ - BmaVariable::new_boolean(3, "var_B", None), - BmaVariable::new(0, "var_A", (1, 3), None), + BmaVariable::new(0, "var_A", (1, 3), None), + BmaVariable::new(3, "var_B", None), ], relationships: vec![ BmaRelationship::new_activator(0, 0, 3), BmaRelationship::new_inhibitor(1, 3, 3), ],Or add documentation explaining the ID mapping:
+// Variable IDs: var_A = 0, var_B = 3 +// Relationships: var_A activates var_B, var_B inhibits itself variables: vec![
38-39: Test layout variable references unknown variable ID.The layout variable with ID 0 references "l_var_A" but the network variable with ID 0 is named "var_A". Similarly, the layout variable with ID 3 is named "l_var_B" but the network variable is "var_B". This naming inconsistency might cause confusion in tests.
Consider aligning the layout variable names with the network variable names:
variables: vec![ - BmaLayoutVariable::new(0, "l_var_A", None), - BmaLayoutVariable::new(3, "l_var_B", Some(13)), + BmaLayoutVariable::new(0, "var_A", None), + BmaLayoutVariable::new(3, "var_B", Some(13)), ],src/serde/json/json_variable.rs (1)
39-62: Consider replacing anyhow::Error with domain-specific error type.The conversion logic is well-implemented with proper error handling for formula parsing. However, the TODO comment on line 40 suggests replacing
anyhow::Errorwith a type-safe error, which would provide better error semantics and type safety.Consider defining a domain-specific error type for conversion failures:
#[derive(Debug, thiserror::Error)] pub enum ConversionError { #[error("Failed to parse formula: {0}")] FormulaParseError(String), #[error("Invalid variable data: {0}")] InvalidVariableData(String), }src/utils.rs (2)
5-12: Improve logic efficiency inis_blankfunction.The current implementation can be simplified by handling the
Nonecase more directly.-pub fn is_blank(value: &Option<String>) -> bool { - if let Some(value) = value { - if value.trim().is_empty() { - return true; - } - } - false -} +pub fn is_blank(value: &Option<String>) -> bool { + value.as_ref().map_or(true, |s| s.trim().is_empty()) +}
45-61: Consider documenting the unusual API design ofis_unique_id.The function returns
Err(())when the value is not found andOk(bool)for uniqueness status. While this design works, it's somewhat unusual and could benefit from clearer documentation or consider using a more conventional return type.Consider a more conventional return type:
pub enum UniquenessResult { NotFound, Unique, Duplicate, } pub fn is_unique_id<T: Eq, ID: Eq, F>( container: &[T], value: &T, id: F ) -> UniquenessResult where F: Fn(&T) -> ID,src/model/layout/bma_layout_container.rs (1)
21-30: Document the rationale for default size value.The default size is set to 1, but it's unclear why this specific value was chosen. Consider adding a comment explaining this choice or referencing any BMA specification that defines this default.
src/serde/json/json_model.rs (1)
65-67: Address the TODO: implement a type-safe error enum.The TODO comment indicates that
anyhow::Errorshould be replaced with a proper error type. This would improve type safety and make error handling more explicit.Would you like me to generate a type-safe error enum for JSON model conversion and open an issue to track this task?
src/model/bma_relationship.rs (1)
83-102: Consider using theis_unique_idutility function for consistency.The manual ID uniqueness check could be simplified by using the same
is_unique_idutility function that's used inBmaVariable::validate_all. This would improve consistency across the codebase.Import and use the utility function:
- // Ensure that the relationship id is unique within the enclosing BmaNetwork. - - let mut count = 0; - let mut found_self = false; - for relationship in &context.relationships { - if relationship.id == self.id { - count += 1; - if relationship == self { - found_self = true; - } - } - } - - assert!( - found_self, - "Validation called on a relationship that is not part of the BmaNetwork" - ); - - if count > 1 { - reporter.report(BmaRelationshipError::IdNotUnique { id: self.id }); - } + // Ensure that the relationship id is unique within the enclosing BmaNetwork. + let Ok(is_unique) = is_unique_id(&context.relationships, self, |x| x.id) else { + panic!("Validation called on a relationship that is not part of the BmaNetwork") + }; + + if !is_unique { + reporter.report(BmaRelationshipError::IdNotUnique { id: self.id }); + }src/model/_impl_bma_model.rs (1)
95-97: Remove or update the outdated TODO comment.The TODO comment states "deal with non-monotonic regulations (ignored for now)", but the implementation at lines 110-131 already handles non-monotonic regulations by adding both activator and inhibitor relationships. This comment should be removed or updated to reflect the current implementation.
- // transform monotonic regulations into relationships, ignore non-monotonic for now - // TODO: deal with non-monotonic regulations (ignored for now) + // transform monotonic regulations into relationships
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
.github/workflows/build.yml(1 hunks).idea/dictionaries/project.xml(1 hunks)Cargo.toml(2 hunks)src/bin/load_json.rs(4 hunks)src/bin/load_xml.rs(1 hunks)src/enums.rs(0 hunks)src/json_model.rs(0 hunks)src/lib.rs(1 hunks)src/model/_impl_bma_model.rs(10 hunks)src/model/_impl_json_serde.rs(0 hunks)src/model/_impl_to_bn.rs(11 hunks)src/model/_impl_xml_serde.rs(6 hunks)src/model/bma_model.rs(1 hunks)src/model/bma_network.rs(1 hunks)src/model/bma_relationship.rs(1 hunks)src/model/bma_variable.rs(1 hunks)src/model/layout/bma_layout.rs(1 hunks)src/model/layout/bma_layout_container.rs(1 hunks)src/model/layout/bma_layout_variable.rs(1 hunks)src/model/layout/mod.rs(1 hunks)src/model/mod.rs(1 hunks)src/serde/json/json_layout.rs(1 hunks)src/serde/json/json_layout_container.rs(1 hunks)src/serde/json/json_layout_variable.rs(1 hunks)src/serde/json/json_model.rs(1 hunks)src/serde/json/json_network.rs(1 hunks)src/serde/json/json_relationship.rs(1 hunks)src/serde/json/json_variable.rs(1 hunks)src/serde/json/mod.rs(1 hunks)src/serde/mod.rs(1 hunks)src/serde/quote_num.rs(1 hunks)src/serde/xml_model.rs(11 hunks)src/update_fn/_impl_to_update_fn.rs(2 hunks)src/update_fn/bma_fn_update.rs(3 hunks)src/update_fn/parser.rs(2 hunks)src/update_fn/tokenizer.rs(3 hunks)src/utils.rs(1 hunks)src/validation.rs(1 hunks)
💤 Files with no reviewable changes (3)
- src/enums.rs
- src/model/_impl_json_serde.rs
- src/json_model.rs
🧰 Additional context used
🧬 Code Graph Analysis (11)
src/bin/load_json.rs (1)
src/model/bma_model.rs (2)
from_bma_json(51-55)to_bma_json(37-41)
src/update_fn/parser.rs (1)
src/update_fn/tokenizer.rs (1)
try_tokenize_bma_formula(23-30)
src/model/mod.rs (5)
src/model/bma_variable.rs (3)
new_boolean(36-38)new(41-53)default(75-82)src/model/bma_network.rs (1)
new(24-30)src/model/layout/bma_layout_variable.rs (1)
new(39-46)src/model/layout/bma_layout_container.rs (2)
new(35-41)default(22-29)src/model/bma_relationship.rs (4)
new_activator(29-36)new_inhibitor(39-46)from(159-164)from(168-173)
src/serde/json/json_layout_container.rs (2)
src/utils.rs (3)
f64_or_default(30-32)rational_or_default(35-37)take_if_not_blank(15-22)src/serde/quote_num.rs (2)
from(11-13)from(17-19)
src/serde/json/json_layout.rs (5)
src/utils.rs (2)
clone_into_vec(25-27)take_if_not_blank(15-22)src/serde/json/json_layout_container.rs (2)
from(24-32)from(36-46)src/serde/json/json_layout_variable.rs (2)
from(37-56)from(60-78)src/serde/json/json_network.rs (1)
from(22-28)src/serde/json/json_model.rs (1)
from(57-62)
src/model/bma_network.rs (5)
src/utils.rs (1)
is_blank(5-12)src/model/bma_variable.rs (3)
new(41-53)validate_all(99-121)default(75-82)src/model/bma_relationship.rs (3)
from(159-164)from(168-173)validate_all(64-102)src/validation.rs (4)
validate_all(71-71)validate_all(94-94)validate(73-81)validate(96-104)src/model/mod.rs (1)
simple_network(20-33)
src/serde/json/json_relationship.rs (1)
src/serde/quote_num.rs (2)
from(11-13)from(17-19)
src/serde/json/json_layout_variable.rs (3)
src/utils.rs (3)
f64_or_default(30-32)rational_or_default(35-37)take_if_not_blank(15-22)src/serde/json/json_layout_container.rs (2)
from(24-32)from(36-46)src/serde/quote_num.rs (2)
from(11-13)from(17-19)
src/model/layout/bma_layout_container.rs (6)
src/utils.rs (2)
is_blank(5-12)is_unique_id(45-61)src/model/bma_variable.rs (2)
new(41-53)validate_all(99-121)src/model/layout/bma_layout_variable.rs (3)
new(39-46)validate_all(82-113)unique_id(217-226)src/model/bma_model.rs (1)
validate_all(68-71)src/model/layout/bma_layout.rs (1)
validate_all(51-63)src/model/bma_relationship.rs (1)
validate_all(64-102)
src/validation.rs (3)
src/model/bma_network.rs (1)
validate_all(57-72)src/model/bma_model.rs (1)
validate_all(68-71)src/model/layout/bma_layout_variable.rs (1)
validate_all(82-113)
src/model/bma_relationship.rs (3)
src/model/bma_network.rs (2)
validate_all(57-72)new(24-30)src/model/bma_model.rs (1)
validate_all(68-71)src/model/bma_variable.rs (3)
validate_all(99-121)new_boolean(36-38)new(41-53)
🔇 Additional comments (57)
src/bin/load_xml.rs (1)
1-1: LGTM: Import update aligns with crate restructuring.The import change correctly reflects the new crate structure while maintaining API compatibility.
.github/workflows/build.yml (1)
17-17:✅ Verification successful
Verify Rust version 1.87.0 exists.
Please confirm that Rust version 1.87.0 is available, as this seems to be a future version that may not exist yet.
🌐 Web query:
What is the latest stable version of Rust and does Rust 1.87.0 exist?💡 Result:
Latest Stable Version of Rust
The latest stable version of Rust as of June 2025 is Rust 1.87.0. This release became available on May 15, 2025, coinciding with the 10-year anniversary of Rust 1.0[1][3][6]. The 1.87.0 release is currently recommended for all users, with subsequent versions in the beta and nightly channels[3].
Does Rust 1.87.0 Exist?
Yes, Rust 1.87.0 exists and is officially recognized as the stable release. It is available for installation using the standard Rust tooling, such as updating with
rustup update stable[1][6][9]. This version introduced several new features and improvements, with detailed release notes published on the official Rust blog and changelog sites[1][6].Summary Table
Attribute Value Latest stable Rust 1.87.0 Release date May 15, 2025 Does 1.87.0 exist? Yes For more details on the changes in Rust 1.87.0, see the official release notes[1][6].
Citations:
- 1: https://blog.rust-lang.org/2025/05/15/Rust-1.87.0/
- 2: https://blog.rust-lang.org/2025/04/03/Rust-1.86.0.html
- 3: https://releases.rs
- 4: https://blog.rust-lang.org
- 5: https://blog.rust-lang.org/2025/01/09/Rust-1.84.0.html
- 6: https://releases.rs/docs/1.87.0/
- 7: https://doc.rust-lang.org/book/appendix-05-editions.html
- 8: https://endoflife.date/rust
- 9: https://www.youtube.com/watch?v=MCg6mBekcjg
- 10: https://forge.rust-lang.org/archive/release-history.html
Rust version update approved: 1.87.0 is valid
Rust 1.87.0 was released on May 15, 2025 and is the current stable version. No changes are needed—the CI workflow’s update is correct.
• File
.github/workflows/build.yml, line 17:RUST_VERSION: "1.87.0"src/update_fn/parser.rs (1)
5-5: LGTM: Import reordering for consistency.The alphabetical ordering of imports improves consistency with the codebase style.
src/model/layout/mod.rs (1)
1-3: LGTM: Clean module structure for layout components.The crate-private visibility and modular organization align well with the refactoring goals of improving project structure and maintainability.
src/update_fn/_impl_to_update_fn.rs (2)
6-6: LGTM! Import reordering improves consistency.The import statement reordering is a good stylistic improvement.
53-53: LGTM! Whitespace cleanup.Removing trailing whitespace improves code cleanliness.
src/serde/mod.rs (1)
1-4: LGTM! Well-organized module structure.The module declarations provide a clean separation of serialization concerns with appropriate crate-level visibility.
src/bin/load_json.rs (2)
1-1: LGTM! Import updated for refactored API.The import change aligns with the crate restructuring mentioned in the PR objectives.
18-18: LGTM! Method calls updated to new API.The method calls have been correctly updated to use the new
from_bma_jsonandto_bma_jsonmethods, maintaining the same functionality with the refactored API.Also applies to: 37-37, 41-41, 55-55
src/update_fn/tokenizer.rs (2)
67-67: LGTM! Syntax fix with semicolon.Adding the semicolon after the error return statement is a good syntax improvement.
284-284: LGTM! Import reordering in tests.The import order change is a stylistic improvement.
Cargo.toml (2)
5-5:✅ Verification successful
Verify Rust edition 2024 availability.
Ensure that Rust edition 2024 is stable and available. If it's not yet released, consider using edition 2021 until 2024 becomes stable.
🌐 Web query:
Is Rust edition 2024 stable and officially released?💡 Result:
Yes, Rust edition 2024 is stable and officially released. The Rust team announced the stabilization of the 2024 Edition alongside Rust version 1.85.0, which was released on February 20, 2025[1][3][4]. Editions in Rust are opt-in and allow for language changes that maintain backward compatibility with existing projects[1][2].
The 2024 Edition brings substantial updates, including numerous improvements to the language, standard library, tooling, and formatting capabilities[3][4][6]. The official documentation and release notes confirm that these changes are now available on the stable channel, meaning all users can adopt and use Rust 2024 for their projects[1][2][6].
To use Rust 2024, update to Rust 1.85.0 or later with the stable toolchain. The edition is fully documented in the Rust Edition Guide and is considered the most feature-rich release so far[2][3].
Citations:
- 1: https://blog.rust-lang.org/2025/02/20/Rust-1.85.0.html
- 2: https://doc.rust-lang.org/edition-guide/rust-2024/index.html
- 3: https://www.developer-tech.com/news/rust-1-85-0-released-2024-edition-stabilised/
- 4: https://www.youtube.com/watch?v=P4-jSupvBFs
- 5: https://news.ycombinator.com/item?id=32945978
- 6: https://doc.rust-lang.org/beta/releases.html
Confirm Rust edition 2024 is stable and ready to use
Rust 1.85.0 (released Feb 20, 2025) officially stabilizes the 2024 Edition. You can safely keep
edition = "2024"—just ensure your CI and contributors upgrade to Rust ≥ 1.85.0.
27-37:❓ Verification inconclusive
Verify dependency versions for security vulnerabilities.
The dependency versions have been updated. Ensure these versions are secure and free from known vulnerabilities.
🏁 Script executed:
#!/bin/bash # Description: Check for security advisories for the updated dependencies echo "Checking security advisories for updated dependencies..." # Check each dependency for security advisories for package in "biodivine-lib-param-bn" "regex" "serde_with" "serde-xml-rs" "num-rational" "num-traits" "thiserror" "anyhow" "rust_decimal"; do echo "Checking $package..." gh api graphql -f query=" { securityVulnerabilities(first: 5, ecosystem: CARGO, package: \"$package\") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }" 2>/dev/null || echo "Unable to check $package" doneLength of output: 8811
Verify Cargo dependency versions for known vulnerabilities
Please confirm that the versions specified in
Cargo.toml(lines 27–37) are free of any security advisories. You can do this by:
- Running a local audit:
cargo install cargo-audit cargo audit- Using GitHub’s GraphQL API (note the correct ecosystem is
CRATES_IO, notCARGO):#!/bin/bash pkgs=( biodivine-lib-param-bn regex serde serde_json serde_with serde-xml-rs num-rational num-traits thiserror anyhow rust_decimal ) for pkg in "${pkgs[@]}"; do echo "Checking $pkg..." gh api graphql -f query=" { securityVulnerabilities(first: 5, ecosystem: CRATES_IO, package: \"$pkg\") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }" 2>/dev/null || echo "Unable to check $pkg" doneReview any reported vulnerabilities and bump to patched versions if needed.
src/serde/json/json_relationship.rs (2)
11-28: Excellent use of serde field aliases for compatibility.The extensive use of serde aliases (
Id/id,FromVariable/fromVariable/fromVariableId/FromVariableId, etc.) demonstrates good forward and backward compatibility planning for different JSON schema variations.
31-51: Clean bidirectional conversion implementations.The
Fromtrait implementations provide clean, straightforward conversions betweenJsonRelationshipandBmaRelationship. The use of.into()forQuoteNumconversions is idiomatic and maintains type safety.src/model/_impl_to_bn.rs (2)
12-19: Enhanced robustness for variables without names.The updated
canonical_var_namemethod now properly handles variables that may not have names by generating canonical names from variable IDs. This is a good defensive programming practice that prevents potential runtime issues.
65-65: Raw identifier usage for keyword field.The field access uses
r#typewhich indicates the field was renamed to use a raw identifier, likely becausetypeis a reserved keyword in Rust. This is the correct approach for handling keyword conflicts.src/serde/json/json_variable.rs (1)
13-25: Well-designed JSON serialization structure.The
JsonVariablestruct appropriately uses serde attributes for flexible field naming (supporting both camelCase and PascalCase) and handles optional fields correctly. The use ofQuoteNumfor numeric fields is a good pattern for handling JSON where numbers might be quoted as strings.src/serde/json/mod.rs (1)
1-16: Clean module organization.The module properly organizes the JSON serialization components and follows standard Rust module organization patterns. The re-exports make the types easily accessible to parent modules.
src/serde/json/json_layout_variable.rs (2)
36-57: LGTM!The conversion implementation correctly handles optional fields and uses appropriate utility functions for type conversions.
59-79: LGTM!The reverse conversion correctly handles the cell tuple splitting and maintains consistency with the forward conversion.
src/update_fn/bma_fn_update.rs (2)
39-46: LGTM!The manual serialization correctly converts the update function to its string representation.
48-59:❌ Incorrect review comment
Verify that formulas don't reference variables by name during deserialization.
The deserialization uses an empty HashMap for the
variablesparameter. According to the function documentation, variables can be referenced by either ID or name. With an empty map, formulas referencing variables by name will fail to parse.Run the following script to check if any formulas in the codebase reference variables by name:
If formulas can reference variables by name, consider modifying the deserialization to accept a variable map or ensure all formulas use IDs only.
🏁 Script executed:
#!/bin/bash # Description: Search for formula strings that might contain variable names instead of IDs # Look for JSON files that might contain formula definitions fd -e json | xargs rg -l "formula|Formula|function|Function" | head -20 # Search for any hardcoded formula strings in the codebase that might use names rg '"[^"]*\b(var|Var)[A-Za-z_]+[^"]*"' --type rust -A 2 -B 2 # Check test files for formula examples fd test | xargs rg "parse_from_str|BmaFnUpdate" -A 5 -B 2 | grep -E '"\w+\s*[+\-*/]|[+\-*/]\s*\w+"'Length of output: 9799
To get clearer insight into whether any formulas reference variables by name, let’s refine our search:
🏁 Script executed:
#!/bin/bash # 1. Find any parse_from_str calls with literal formula strings in code or tests rg 'parse_from_str\(\s*"' -n # 2. Search all JSON model files for "formula" fields rg -g '*.json' -n '"formula"\s*:' | head -30 # 3. Look for formula-like string literals in Rust source (e.g. var_A + var_B) rg -n '"[A-Za-z_]+\s*[\+\-\*/]' --type rust # 4. Check where BmaFnUpdate is exercised in tests rg -n 'BmaFnUpdate::parse_from_str' -nLength of output: 45514
To confirm whether the manual
Deserializeimpl inbma_fn_update.rsis actually used when loading JSON and not overridden by custom logic injson_variable.rs, let’s inspect how theupdate_functionfield is deserialized there:
🏁 Script executed:
#!/bin/bash # Locate and inspect how JsonVariable deserializes `update_function` file=$(fd json_variable.rs | head -1) echo "Inspecting: $file" rg -n "update_function" -A5 -B5 $fileLength of output: 233
Let’s inspect how
JsonVariableusesBmaFnUpdateinjson_variable.rs:
🏁 Script executed:
#!/bin/bash # Show context around parse_from_str in json_variable.rs rg -n "BmaFnUpdate::parse_from_str" -A5 -B5 src/serde/json/json_variable.rs # Show the JsonVariable struct and its formula/update field sed -n '1,80p' src/serde/json/json_variable.rsLength of output: 2707
Ignore
Deserializeimpl on BmaFnUpdate – it isn’t used for JSON/XML parsingJSON and XML deserialization bypass that impl by calling
BmaFnUpdate::parse_from_str(..., &variables)with a real name-to-ID map (seejson_variable.rsand_impl_xml_serde.rs). The blanketDefault::default()path is never invoked when loading model formulas, so there’s no risk of name-based references failing.Likely an incorrect or invalid review comment.
src/serde/json/json_layout.rs (1)
1-41: LGTM!The JSON layout serialization module is well-structured with proper default handling and consistent use of utility functions.
src/serde/json/json_layout_container.rs (3)
1-21: Well-structured JSON serialization with proper field handling.The
JsonLayoutContainerstruct is well-designed with appropriate serde attributes for flexible JSON key handling. The use of#[serde(default)]for optional fields and alias support for both PascalCase and camelCase variants provides good compatibility.
23-33: Conversion logic looks correct.The conversion from
BmaLayoutContainertoJsonLayoutContainerproperly handles optional fields and uses the appropriate utility functions for type conversions.
35-47: Good use of utility function for blank string handling.The reverse conversion correctly uses
take_if_not_blankto filter out empty names, which aligns with the validation logic seen in other parts of the codebase.src/serde/json/json_network.rs (3)
1-19: Clean JSON network structure with proper field handling.The
JsonNetworkstruct is well-designed with clear documentation and appropriate serde attributes. The field naming conventions are consistent with the rest of the codebase.
21-29: Straightforward conversion implementation.The conversion from
BmaNetworktoJsonNetworkproperly handles optional fields and uses theclone_into_vecutility for converting collections.
31-47: Proper error handling in reverse conversion.The
TryFromimplementation correctly propagates errors from variable conversions and usesanyhow::Errorfor unified error handling. The use ofcollect::<Result<Vec<_>, _>>()properly short-circuits on the first error.src/model/bma_network.rs (5)
10-20: Well-documented core network structure.The
BmaNetworkstruct is clearly documented and uses appropriate serde attributes. The structure logically represents a network with variables and relationships.
22-41: Useful utility methods with clear naming.The constructor and utility methods provide clear interfaces for creating networks and finding variables. The
name_or_defaultmethod provides a sensible fallback for display purposes.
43-52: Proper error handling with transparent wrapping.The error enum correctly uses
#[error(transparent)]for wrapping component errors and provides clear error messages. This follows Rust error handling best practices.
54-73: Comprehensive validation implementation.The validation logic correctly checks for empty names using the
is_blankutility and properly delegates validation to contained components. The use ofreporter.wrap()ensures proper error propagation.
75-101: Good test coverage for validation scenarios.The tests cover the key validation scenarios including valid networks and the empty name error case. The use of test helpers from the
testsmodule promotes consistency.src/model/layout/bma_layout.rs (5)
11-23: Well-structured layout model with clear documentation.The
BmaLayoutstruct is properly documented and uses appropriate optional fields for layout information. The use ofRational64for coordinates provides precision for positioning data.
25-35: Consistent find methods for layout components.The
find_variableandfind_containermethods follow the same pattern as the network'sfind_variablemethod, providing consistent API design across the codebase.
37-46: Proper error wrapping for layout validation.The error enum correctly wraps errors from contained components and provides clear error messages. The structure follows the same pattern as
BmaNetworkError.
48-64: Correct contextual validation implementation.The validation logic properly checks for empty descriptions and delegates validation to contained components. Note that containers are validated against the layout context (
self) while variables are validated against the model context.
66-108: Comprehensive test coverage with proper context setup.The tests correctly set up the full model context required for validation and cover both valid and invalid scenarios. The test structure is consistent with other validation tests in the codebase.
src/validation.rs (1)
1-106: Well-designed validation framework!The error reporter abstraction with type conversion support and the separation between contextual and non-contextual validation provides a flexible and extensible validation system. The implementation follows good Rust patterns with proper trait bounds and error handling.
src/model/bma_model.rs (1)
1-168: Excellent refactoring with improved modularity and error handling!The changes successfully modularize the BMA model structure by:
- Renaming
modeltonetworkfor clarity- Implementing clean JSON serialization through intermediate types
- Providing comprehensive error handling with
BmaModelError- Delegating validation to components using the new framework
- Adding thorough test coverage including complex error scenarios
The implementation follows SOLID principles and maintains good separation of concerns.
src/model/layout/bma_layout_variable.rs (1)
1-228: Well-structured layout variable implementation!The implementation provides:
- Clear documentation of invariants
- Comprehensive validation logic checking all constraints
- Good test coverage for various error scenarios
- Proper use of the
ContextualValidationtraitThe struct design with optional fields and the
VariableTypeenum is clean and follows Rust best practices.src/model/bma_relationship.rs (2)
116-174: Well-implemented enum with proper serialization and conversions.The
RelationshipTypeenum implementation is clean with:
- Proper custom serialization for XML/JSON compatibility
- Case-insensitive deserialization for robustness
- Bidirectional conversions with
Monotonicity
176-286: Excellent test coverage!The tests comprehensively cover:
- Variable lookup functionality
- Type conversions
- Serialization/deserialization
- All validation error cases
- Edge cases including panic conditions
This provides confidence in the implementation's correctness.
src/serde/xml_model.rs (2)
26-29: Verify the breaking change in XML format is intentional.The change from XML elements to attributes for
idandnamefields (using@Idand@Name) represents a breaking change in the XML format. While the aliases provide backward compatibility, this should be documented if it's an intentional format migration.Please confirm this XML format change aligns with the project's migration strategy and that existing XML files will be properly handled.
54-56: Excellent refactoring with unifiedQuoteNumtype.The consistent use of
QuoteNumthroughout the file provides a clean, unified approach to handling quoted numeric values in XML. The conversions tou32in the implementation methods are properly handled.Also applies to: 80-81, 88-88, 112-118, 131-135, 155-159, 163-163, 172-172, 181-182
src/model/_impl_bma_model.rs (2)
14-20: Clean refactoring to use the new modular structure.The renaming from
modeltonetworkis consistently applied, and the code properly uses the new accessor methods (min_level(),max_level()) and field names (r#type). The implementation maintains the same logic while adapting to the new structure.Also applies to: 30-33, 43-56
57-165: Well-implemented conversion from BooleanNetwork.The
from_boolean_networkmethod correctly:
- Validates that the network has no parameters
- Constructs variables with proper optional fields
- Handles both monotonic and non-monotonic regulations appropriately
- Creates a sensible default layout
- Maintains deterministic ordering of relationships
src/model/_impl_xml_serde.rs (3)
1-8: LGTM! Import changes align with the refactoring.The updated imports reflect the modular restructuring and introduction of precise numeric handling with
Rational64.
30-30: LGTM! Type-safe ID conversion.The
.into()conversion suggests stronger typing for IDs, which improves type safety.
48-50: LGTM! Proper optional field handling and type conversions.The changes correctly wrap the name in
Some()and use.into()for type-safe conversions.src/lib.rs (5)
8-8: Good practice: Crate-private serde module.Making the
serdemodule crate-private while exposing functionality through the main types is a good encapsulation practice.
10-18: Excellent API design with comprehensive re-exports.The public re-exports provide a clean, flat API surface while maintaining internal modularity. This follows Rust best practices by:
- Exposing both the main types and their associated error types
- Maintaining backward compatibility through re-exports
- Keeping internal module structure flexible for future changes
20-23: Good encapsulation of validation framework.The validation module is appropriately private with selective public re-exports of the validation traits and types. This aligns well with the PR objective of improving error handling.
25-25: Appropriate visibility for utility module.Making the
utilsmodule crate-private is correct since utility functions are typically internal implementation details.
4-4:✅ Verification successful
Breaking change: Verify impact of making model module crate-private.
Changing
pub mod modeltopub(crate) mod modelis a breaking change that will affect external users who directly access the module path. However, since the types are re-exported below, the impact should be minimal if users import types directly.
🏁 Script executed:
#!/bin/bash # Search for any direct module path usage that might be affected by this change rg -A 3 -B 1 "model::" --type rust rg -A 3 -B 1 "::model::" --type rustLength of output: 7229
Public API remains stable after
modelmodule visibility change
- All internal references to
crate::modelare confined within the crate (tests and submodules).- Every public type from
modelis re-exported insrc/lib.rs, so external users will continue to import via the top-level API (e.g.,your_crate::BmaModel).No breaking changes detected.
| if variables.contains_key(&id) { | ||
| return Ok(id); | ||
| } | ||
| return Ok(id); |
There was a problem hiding this comment.
Potential issue: Removed ID validation may allow invalid variable references.
The removal of the check to verify that a parsed ID exists in the variables map could allow referencing non-existent variables. This may lead to runtime errors later when the ID is used.
Consider restoring the validation:
if let Ok(id) = var_string.parse::<u32>() {
+ if !variables.contains_key(&id) {
+ return Err(format!("Variable ID {id} not found in variables map"));
+ }
return Ok(id);
}📝 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.
| return Ok(id); | |
| if let Ok(id) = var_string.parse::<u32>() { | |
| if !variables.contains_key(&id) { | |
| return Err(format!("Variable ID {id} not found in variables map")); | |
| } | |
| return Ok(id); | |
| } |
🤖 Prompt for AI Agents
In src/update_fn/tokenizer.rs at line 146, the code returns an ID without
validating if it exists in the variables map, which can cause runtime errors.
Restore the validation by checking if the parsed ID is present in the variables
map before returning it. If the ID is not found, return an appropriate error to
prevent invalid variable references.
| impl<'de> Deserialize<'de> for QuoteNum { | ||
| fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| let s: String = String::deserialize(deserializer)?; | ||
| let trimmed = s.trim_matches('"'); | ||
| let value = u32::from_str(trimmed).map_err(serde::de::Error::custom)?; | ||
| Ok(Self(value)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical issue: Deserializer fails on unquoted numbers.
The current Deserialize implementation assumes all numeric input is a string, but JSON can contain both quoted strings ("123") and unquoted numbers (123). This will cause deserialization failures when encountering normal JSON numbers.
Consider using serde_with::formats::Flexible or implement a custom deserializer that can handle both cases:
impl<'de> Deserialize<'de> for QuoteNum {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
- let s: String = String::deserialize(deserializer)?;
- let trimmed = s.trim_matches('"');
- let value = u32::from_str(trimmed).map_err(serde::de::Error::custom)?;
+ use serde::de::{self, Visitor};
+ use std::fmt;
+
+ struct QuoteNumVisitor;
+
+ impl<'de> Visitor<'de> for QuoteNumVisitor {
+ type Value = QuoteNum;
+
+ fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+ formatter.write_str("a number or a quoted number string")
+ }
+
+ fn visit_u32<E>(self, value: u32) -> Result<Self::Value, E>
+ where
+ E: de::Error,
+ {
+ Ok(QuoteNum(value))
+ }
+
+ fn visit_u64<E>(self, value: u64) -> Result<Self::Value, E>
+ where
+ E: de::Error,
+ {
+ if value <= u32::MAX as u64 {
+ Ok(QuoteNum(value as u32))
+ } else {
+ Err(E::custom(format!("number {} is too large for u32", value)))
+ }
+ }
+
+ fn visit_str<E>(self, value: &str) -> Result<Self::Value, E>
+ where
+ E: de::Error,
+ {
+ let trimmed = value.trim_matches('"');
+ let parsed = u32::from_str(trimmed).map_err(E::custom)?;
+ Ok(QuoteNum(parsed))
+ }
+ }
+
+ deserializer.deserialize_any(QuoteNumVisitor)
- Ok(Self(value))
}
}🤖 Prompt for AI Agents
In src/serde/quote_num.rs around lines 31 to 41, the Deserialize implementation
for QuoteNum incorrectly assumes the input is always a quoted string, causing
failures on unquoted numeric JSON values. To fix this, update the deserializer
to accept both quoted strings and unquoted numbers by implementing a custom
deserialization that tries to parse the input as a u32 directly or from a
string, or alternatively use serde_with::formats::Flexible to handle both
formats seamlessly.
| pub id: QuoteNum, | ||
| #[serde( | ||
| rename = "FromVariable", | ||
| alias = "fromVariable", | ||
| alias = "fromVariableId", | ||
| alias = "FromVariableId" | ||
| )] | ||
| pub from_variable: QuoteNum, | ||
| #[serde( | ||
| rename = "ToVariable", | ||
| alias = "toVariable", | ||
| alias = "toVariableId", | ||
| alias = "ToVariableId" | ||
| )] | ||
| pub to_variable: QuoteNum, |
There was a problem hiding this comment.
Potential deserialization issues due to QuoteNum implementation.
The numeric fields use QuoteNum which has a critical issue in its Deserialize implementation (see previous comment on quote_num.rs). This will cause failures when deserializing JSON with unquoted numbers.
This issue needs to be resolved in the QuoteNum implementation before this module will work correctly with standard JSON numeric fields.
🤖 Prompt for AI Agents
In src/serde/json/json_relationship.rs lines 12 to 26, the fields use the
QuoteNum type which has a flawed Deserialize implementation causing failures
when deserializing unquoted JSON numbers. To fix this, first correct the
Deserialize implementation in the QuoteNum type to properly handle both quoted
and unquoted numeric JSON values. Only after fixing QuoteNum should this module
be used to ensure reliable deserialization of numeric fields.
| let mut variables_sorted = self.network.variables.clone(); | ||
| variables_sorted.sort_by_key(|var| (var.id, var.name.clone())); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify consistency of refactored field access.
The code correctly updates field access from self.model to self.network to align with the internal structure refactoring. Ensure that all usages of the old field names have been consistently updated throughout the codebase.
Also applies to: 53-54, 162-163, 172-172
🏁 Script executed:
#!/bin/bash
# Description: Verify no remaining references to old field names
# Expected: No matches for the old field access patterns
echo "Checking for any remaining references to old field names..."
rg "self\.model\.(variables|relationships)" --type rust
echo "Checking for direct field access that should use methods..."
rg "\.range\s*\." --type rust -A 2Length of output: 1091
Address remaining direct .range field accesses
We’ve confirmed there are no lingering self.model.(variables|relationships) references, but the refactor introduced new getter methods for range, so direct tuple‐field access should be replaced. Update the following locations to use the appropriate range() (or min_/max_ accessors) methods instead of .range.0/.range.1:
- src/serde/json/json_variable.rs
range_from: value.range.0.into()range_to: value.range.1.into()
- src/model/bma_variable.rs
self.range.0self.range.1if self.range.0 > self.range.1 { … }
After making these changes, re-run your grep script to ensure no direct .range accesses remain.
🤖 Prompt for AI Agents
In src/model/_impl_to_bn.rs around lines 40-41 and also at lines 53-54, 162-163,
and 172, replace all direct tuple field accesses of `.range.0` and `.range.1`
with the appropriate getter methods such as `range().0` and `range().1` or
dedicated `min_` and `max_` accessors. This ensures consistency with the new
encapsulation of the `range` field. After updating these, verify no direct
`.range` tuple accesses remain by re-running the grep script.
| #[serde(rename = "CellX", alias = "cellX")] | ||
| pub cell_x: Option<QuoteNum>, | ||
| #[serde(rename = "CellY", alias = "cellY")] | ||
| pub cell_y: Option<QuoteNum>, |
There was a problem hiding this comment.
Add missing default attributes to optional cell coordinate fields.
The documentation states that cell coordinates are optional, but the cell_x and cell_y fields are missing the default attribute. This will cause deserialization to fail if these fields are not present in the JSON.
Apply this diff to fix the issue:
- #[serde(rename = "CellX", alias = "cellX")]
+ #[serde(default, rename = "CellX", alias = "cellX")]
pub cell_x: Option<QuoteNum>,
- #[serde(rename = "CellY", alias = "cellY")]
+ #[serde(default, rename = "CellY", alias = "cellY")]
pub cell_y: Option<QuoteNum>,📝 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.
| #[serde(rename = "CellX", alias = "cellX")] | |
| pub cell_x: Option<QuoteNum>, | |
| #[serde(rename = "CellY", alias = "cellY")] | |
| pub cell_y: Option<QuoteNum>, | |
| #[serde(default, rename = "CellX", alias = "cellX")] | |
| pub cell_x: Option<QuoteNum>, | |
| #[serde(default, rename = "CellY", alias = "cellY")] | |
| pub cell_y: Option<QuoteNum>, |
🤖 Prompt for AI Agents
In src/serde/json/json_layout_variable.rs around lines 30 to 33, the optional
fields cell_x and cell_y lack the serde default attribute, causing
deserialization to fail if these fields are missing in the JSON input. Add
#[serde(default)] above both cell_x and cell_y fields to ensure they default to
None when absent during deserialization.
| fn convert_xml_layout_var(xml_var: XmlVariable) -> BmaLayoutVariable { | ||
| let cell = if let (Some(x), Some(y)) = (xml_var.cell_x, xml_var.cell_y) { | ||
| Some((u32::from(x), u32::from(y))) | ||
| } else { | ||
| None | ||
| }; | ||
| BmaLayoutVariable { | ||
| id: xml_var.id, | ||
| name: xml_var.name, | ||
| variable_type: xml_var.r#type, | ||
| container_id: xml_var.container_id, | ||
| position_x: xml_var.position_x.unwrap_or_default(), | ||
| position_y: xml_var.position_y.unwrap_or_default(), | ||
| cell_x: xml_var.cell_x, | ||
| cell_y: xml_var.cell_y, | ||
| angle: xml_var.angle.unwrap_or_default(), | ||
| description: String::default(), | ||
| id: xml_var.id.into(), | ||
| name: Some(xml_var.name), | ||
| r#type: xml_var.r#type, | ||
| container_id: xml_var.container_id.map(|it| it.into()), | ||
| position: ( | ||
| Rational64::from_f64(xml_var.position_x.unwrap_or_default()).unwrap(), | ||
| Rational64::from_f64(xml_var.position_y.unwrap_or_default()).unwrap(), | ||
| ), | ||
| cell, | ||
| angle: Rational64::from_f64(xml_var.angle.unwrap_or_default()).unwrap(), | ||
| description: None, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update function signature to return Result for error propagation.
Since the function now needs to handle potential errors from Rational64 conversions, the signature should be updated to propagate errors properly.
- fn convert_xml_layout_var(xml_var: XmlVariable) -> BmaLayoutVariable {
+ fn convert_xml_layout_var(xml_var: XmlVariable) -> Result<BmaLayoutVariable, String> {
let cell = if let (Some(x), Some(y)) = (xml_var.cell_x, xml_var.cell_y) {
Some((u32::from(x), u32::from(y)))
} else {
None
};
- BmaLayoutVariable {
+ Ok(BmaLayoutVariable {
id: xml_var.id.into(),
name: Some(xml_var.name),
r#type: xml_var.r#type,
container_id: xml_var.container_id.map(|it| it.into()),
position: (
Rational64::from_f64(xml_var.position_x.unwrap_or_default())
.ok_or("Invalid position_x value")?,
Rational64::from_f64(xml_var.position_y.unwrap_or_default())
.ok_or("Invalid position_y value")?,
),
cell,
angle: Rational64::from_f64(xml_var.angle.unwrap_or_default())
.ok_or("Invalid angle value")?,
description: None,
- }
+ })
}🤖 Prompt for AI Agents
In src/model/_impl_xml_serde.rs lines 66 to 85, update the function signature of
convert_xml_layout_var to return a Result type to handle potential errors from
Rational64::from_f64 conversions. Modify the function body to propagate errors
using the ? operator or equivalent error handling, ensuring that any failure in
converting floats to Rational64 is properly returned as an error instead of
unwrapping.
| Rational64::from_f64(xml_container.position_x).unwrap(), | ||
| Rational64::from_f64(xml_container.position_y).unwrap(), | ||
| ), |
There was a problem hiding this comment.
Replace .unwrap() calls with proper error handling.
Similar to the previous issue, these .unwrap() calls could cause panics with invalid floating point values.
- position: (
- Rational64::from_f64(xml_container.position_x).unwrap(),
- Rational64::from_f64(xml_container.position_y).unwrap(),
- ),
+ position: (
+ Rational64::from_f64(xml_container.position_x)
+ .ok_or("Invalid container position_x value")?,
+ Rational64::from_f64(xml_container.position_y)
+ .ok_or("Invalid container position_y value")?,
+ ),Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/model/_impl_xml_serde.rs around lines 94 to 96, the use of .unwrap() on
Rational64::from_f64 can cause panics if the floating point values are invalid.
Replace these .unwrap() calls with proper error handling by checking if the
conversion returns Some and handling the None case gracefully, such as returning
a Result with an error or using a fallback value, to avoid runtime panics.
| Rational64::from_f64(xml_var.position_x.unwrap_or_default()).unwrap(), | ||
| Rational64::from_f64(xml_var.position_y.unwrap_or_default()).unwrap(), | ||
| ), | ||
| cell, | ||
| angle: Rational64::from_f64(xml_var.angle.unwrap_or_default()).unwrap(), |
There was a problem hiding this comment.
Replace .unwrap() calls with proper error handling.
The .unwrap() calls after Rational64::from_f64() could cause panics if the XML contains invalid floating point values (NaN, infinity, or values that can't be represented as rational numbers).
- position: (
- Rational64::from_f64(xml_var.position_x.unwrap_or_default()).unwrap(),
- Rational64::from_f64(xml_var.position_y.unwrap_or_default()).unwrap(),
- ),
+ position: (
+ Rational64::from_f64(xml_var.position_x.unwrap_or_default())
+ .ok_or("Invalid position_x value")?,
+ Rational64::from_f64(xml_var.position_y.unwrap_or_default())
+ .ok_or("Invalid position_y value")?,
+ ),- angle: Rational64::from_f64(xml_var.angle.unwrap_or_default()).unwrap(),
+ angle: Rational64::from_f64(xml_var.angle.unwrap_or_default())
+ .ok_or("Invalid angle value")?,🤖 Prompt for AI Agents
In src/model/_impl_xml_serde.rs around lines 78 to 82, the code uses `.unwrap()`
on the result of `Rational64::from_f64()`, which can panic if the float is
invalid. Replace these `.unwrap()` calls with proper error handling by checking
if the conversion returns `Some` or `None`. If `None`, handle the error
gracefully, such as returning a Result with an error or providing a default
fallback value, to avoid panics on invalid input.
| let zoom_level = Rational64::from_f64(layout.zoom_level).unwrap(); | ||
| let pan_x = Rational64::from_f64(layout.pan_x).unwrap(); | ||
| let pan_y = Rational64::from_f64(layout.pan_y).unwrap(); |
There was a problem hiding this comment.
Replace .unwrap() calls with proper error handling.
The same issue exists here with potential panics from invalid floating point conversions.
- let zoom_level = Rational64::from_f64(layout.zoom_level).unwrap();
- let pan_x = Rational64::from_f64(layout.pan_x).unwrap();
- let pan_y = Rational64::from_f64(layout.pan_y).unwrap();
+ let zoom_level = Rational64::from_f64(layout.zoom_level)
+ .ok_or("Invalid zoom_level value")?;
+ let pan_x = Rational64::from_f64(layout.pan_x)
+ .ok_or("Invalid pan_x value")?;
+ let pan_y = Rational64::from_f64(layout.pan_y)
+ .ok_or("Invalid pan_y value")?;📝 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 zoom_level = Rational64::from_f64(layout.zoom_level).unwrap(); | |
| let pan_x = Rational64::from_f64(layout.pan_x).unwrap(); | |
| let pan_y = Rational64::from_f64(layout.pan_y).unwrap(); | |
| let zoom_level = Rational64::from_f64(layout.zoom_level) | |
| .ok_or("Invalid zoom_level value")?; | |
| let pan_x = Rational64::from_f64(layout.pan_x) | |
| .ok_or("Invalid pan_x value")?; | |
| let pan_y = Rational64::from_f64(layout.pan_y) | |
| .ok_or("Invalid pan_y value")?; |
🤖 Prompt for AI Agents
In src/model/_impl_xml_serde.rs around lines 131 to 133, the use of .unwrap() on
Rational64::from_f64 calls can cause panics if the floating point values are
invalid. Replace these .unwrap() calls with proper error handling by checking if
the conversion returns Some value and handling the None case gracefully, such as
returning a Result error or using a fallback value, to avoid runtime panics.
| fn convert_xml_container(xml_container: XmlContainer) -> BmaLayoutContainer { | ||
| BmaLayoutContainer { | ||
| id: xml_container.id.into(), | ||
| name: Some(xml_container.name), | ||
| size: xml_container.size.into(), | ||
| position: ( | ||
| Rational64::from_f64(xml_container.position_x).unwrap(), | ||
| Rational64::from_f64(xml_container.position_y).unwrap(), | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update function signature to return Result for error propagation.
This function also needs to return a Result to handle potential Rational64 conversion errors.
- fn convert_xml_container(xml_container: XmlContainer) -> BmaLayoutContainer {
+ fn convert_xml_container(xml_container: XmlContainer) -> Result<BmaLayoutContainer, String> {
- BmaLayoutContainer {
+ Ok(BmaLayoutContainer {
id: xml_container.id.into(),
name: Some(xml_container.name),
size: xml_container.size.into(),
position: (
Rational64::from_f64(xml_container.position_x)
.ok_or("Invalid container position_x value")?,
Rational64::from_f64(xml_container.position_y)
.ok_or("Invalid container position_y value")?,
),
- }
+ })
}📝 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.
| fn convert_xml_container(xml_container: XmlContainer) -> BmaLayoutContainer { | |
| BmaLayoutContainer { | |
| id: xml_container.id.into(), | |
| name: Some(xml_container.name), | |
| size: xml_container.size.into(), | |
| position: ( | |
| Rational64::from_f64(xml_container.position_x).unwrap(), | |
| Rational64::from_f64(xml_container.position_y).unwrap(), | |
| ), | |
| } | |
| } | |
| fn convert_xml_container(xml_container: XmlContainer) -> Result<BmaLayoutContainer, String> { | |
| Ok(BmaLayoutContainer { | |
| id: xml_container.id.into(), | |
| name: Some(xml_container.name), | |
| size: xml_container.size.into(), | |
| position: ( | |
| Rational64::from_f64(xml_container.position_x) | |
| .ok_or("Invalid container position_x value")?, | |
| Rational64::from_f64(xml_container.position_y) | |
| .ok_or("Invalid container position_y value")?, | |
| ), | |
| }) | |
| } |
🤖 Prompt for AI Agents
In src/model/_impl_xml_serde.rs around lines 88 to 98, the function
convert_xml_container currently returns BmaLayoutContainer directly but uses
unwrap() on Rational64::from_f64 conversions which can fail. Change the function
signature to return Result<BmaLayoutContainer, SomeErrorType> and replace
unwrap() calls with proper error handling to propagate conversion errors instead
of panicking.
I'm experimenting with proposing some new "best practices" for Sybila Rust projects, especially in terms of error handling. This PR is a playground where I try to migrate this project to these "best practices". Nothing much has changed in terms of functionality, but the public API is quite different... a lot of things can still change.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores