Add support for parsing and analysing negative values #9
Conversation
WalkthroughThe recent changes introduce a new module for simplifying expressions, handle negative values more effectively, and adjust the parser to support unsigned integers and negative expressions. Tests were added to ensure correct handling of various arithmetic operations. Updates to the Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 8
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- README.md (1 hunks)
- src/analyzer.rs (2 hunks)
- src/analyzer/analyze_rounding.rs (1 hunks)
- src/analyzer/ast.rs (2 hunks)
- src/analyzer/simplify_expr.rs (1 hunks)
- src/parser/arithmetic.lalrpop (2 hunks)
- src/parser/formula_config.rs (1 hunks)
- src/parser/input.rs (1 hunks)
- src/printer/latex_generator.rs (1 hunks)
Additional Context Used
LanguageTool (3)
README.md (3)
Near line 14: Possible missing article found.
Context: ...direction of the substracted element is inverse of the expected rounding) - `A * B => r...
Rule ID: AI_HYDRA_LEO_MISSING_THE
Near line 25: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...to generate user configuration file. - Runroundme analyzeto analyze the config...
Rule ID: ENGLISH_WORD_REPEAT_BEGINNING_RULE
Near line 40: The past participle is required after “to be”, alternatively you could omit the “be”.
Context: ...``` -formulacontains the formula to be analyze. All the numeric literals and identifie...
Rule ID: BE_VBP_IN
Markdownlint (10)
README.md (10)
23: Expected: 0 or 2; Actual: 1
Trailing spaces
24: Expected: 0 or 2; Actual: 1
Trailing spaces
5: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
34: null
Fenced code blocks should be surrounded by blank lines
39: null
Fenced code blocks should be surrounded by blank lines
50: null
Fenced code blocks should be surrounded by blank lines
55: null
Fenced code blocks should be surrounded by blank lines
6: null
Lists should be surrounded by blank lines
40: null
Lists should be surrounded by blank lines
15: null
Spaces inside code span elements
Additional comments not posted (9)
src/parser/arithmetic.lalrpop (1)
30-31: Ensure the new grammar rules for negative numbers and expressions are thoroughly tested.Also applies to: 33-34, 35-35
src/parser/formula_config.rs (1)
30-34: The new constructor method forFormulaConfigenhances flexibility but ensure it is covered by unit tests.Verification successful
The search results indicate multiple instances of
FormulaConfig::newinsrc/analyzer.rs, but it is unclear if these are within unit tests. To ensure the constructor method is adequately tested, we need to verify if these instances are part of test functions.
The instances of
FormulaConfig::neware indeed within test functions insrc/analyzer.rs. This confirms that the new constructor method forFormulaConfigis covered by unit tests.
- Instances of
FormulaConfig::neware found within multiple test functions insrc/analyzer.rs.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that there are unit tests covering the new constructor method for `FormulaConfig`. # Test: Search for unit tests related to the new constructor method. Expect: At least one test. rg --type rust 'test.*new'Length of output: 28
Script:
#!/bin/bash # Description: Verify that there are unit tests covering the new constructor method for `FormulaConfig`. # Step 1: Locate the definition of the `FormulaConfig` struct. echo "Searching for the definition of the 'FormulaConfig' struct..." rg --type rust 'struct FormulaConfig' # Step 2: Search for any instantiation of `FormulaConfig` in test files. echo "Searching for instantiations of 'FormulaConfig' in test files..." rg --type rust 'FormulaConfig::new'Length of output: 1105
Script:
#!/bin/bash # Description: Verify that the instances of `FormulaConfig::new` in `src/analyzer.rs` are within unit tests. # Step 1: Search for test functions in `src/analyzer.rs`. echo "Searching for test functions in 'src/analyzer.rs'..." rg --type rust '#\[test\]' src/analyzer.rs # Step 2: Extract lines around the instances of `FormulaConfig::new` to check if they are within test functions. echo "Extracting context around 'FormulaConfig::new' instances in 'src/analyzer.rs'..." rg --type rust -C 5 'FormulaConfig::new' src/analyzer.rsLength of output: 3089
src/analyzer/simplify_expr.rs (2)
57-65: Assess thesimplify_divfunction for correctness, especially in handling negative values and rounding.
47-55: Ensure thesimplify_mulfunction handles rounding correctly and is covered by unit tests.Verification successful
The search results indicate that there are multiple test functions related to
Opcode::Muland other operations in thesrc/analyzer.rsandsrc/analyzer/ast.rsfiles. These tests likely cover the functionality of thesimplify_mulfunction indirectly through the expression simplification process.
src/analyzer.rscontains tests such astest_mul_up,test_mul_down,test_negative, andtest_double_negative, which likely involve multiplication operations.src/analyzer/ast.rscontains tests liketest_display_opcode_mul_init,test_display_opcode_mul_up,test_display_opcode_mul_down, andtest_display_opcode_mul_unknown, which test different rounding modes for multiplication.Given this context, it appears that the
simplify_mulfunction is covered by these tests.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that there are unit tests covering the `simplify_mul` function. # Test: Search for unit tests related to `simplify_mul`. Expect: At least one test. rg --type rust 'test.*simplify_mul'Length of output: 37
Script:
#!/bin/bash # Locate the `simplify_mul` function definition to confirm its location. rg --type rust 'fn simplify_mul' # Search for any test files or modules that might contain relevant tests. fd --type f --extension rs --exec rg 'simplify_mul' {}Length of output: 384
Script:
#!/bin/bash # Extract the context of `simplify_mul` usage within `src/analyzer/simplify_expr.rs`. rg --context 5 'simplify_mul' src/analyzer/simplify_expr.rs # Search for test files or modules that might contain relevant tests, focusing on `Opcode::Mul`. fd --type f --extension rs --exec rg 'Opcode::Mul' {}Length of output: 1554
Script:
#!/bin/bash # Search for test functions that might cover the expression simplification process. rg --type rust 'fn test' --context 5 # Look for any test files or modules that might contain relevant tests. fd --type f --extension rs --exec rg 'test' {}Length of output: 11002
src/analyzer.rs (2)
2-2: The integration ofsimplify_exprand its usage inanalyzefunction are well-implemented. The changes are consistent with the PR's objectives to handle negative values.Also applies to: 17-21
24-147: The test cases are comprehensive and effectively validate the handling of negative values and rounding directions. Good coverage of different scenarios.src/analyzer/ast.rs (2)
10-10: Changing theNumbervariant fromi32tou32is a thoughtful change, ensuring that negative numbers are handled explicitly using theNegativevariant.
13-14: The addition of theNegativevariant to theExprenum and its handling in theDisplayimplementation are well-executed. This change supports the explicit handling of negative values, aligning with the PR's objectives.Also applies to: 68-68
src/printer/latex_generator.rs (1)
28-28: The handling ofExpr::Negativein the LaTeX generator is correctly implemented, ensuring that negative values are properly formatted in the output documents.
| Num: i32 = { | ||
| r"[0-9]+" =>? i32::from_str(<>) | ||
| Num: u32 = { | ||
| r"[0-9]+" =>? u32::from_str(<>) |
There was a problem hiding this comment.
Tip
Codebase Verification
Add tests for parsing large numbers to ensure error handling is properly tested.
- The parser code includes error handling for large numbers using
u32::from_strand maps errors to a "number is too big" message. - However, there are no tests specifically for parsing large numbers.
Consider adding tests to cover this scenario.
Analysis chain
Validate the error handling for large numbers during parsing.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the error handling for large numbers is correctly implemented and tested.
# Test: Search for unit tests related to error handling for large numbers. Expect: At least one test.
rg --type rust 'test.*"number is too big"'
Length of output: 44
Script:
#!/bin/bash
# Description: Verify the presence of general number parsing tests and error handling for large numbers.
# Step 1: Search for general number parsing tests.
rg --type rust 'test.*number'
# Step 2: Examine the parser code for error handling related to large numbers.
rg --type rust 'u32::from_str' src/parser/arithmetic.lalrpop
# Step 3: Check for any relevant documentation or comments in the parser code.
rg --type rust 'large number' src/parser/arithmetic.lalrpop
Length of output: 241
Script:
#!/bin/bash
# Description: Further investigate the handling of number parsing and error handling for large numbers.
# Step 1: Extract and review the `test_display_number` function.
rg --type rust 'fn test_display_number' -A 20
# Step 2: Search for error handling mechanisms in the parser code where `u32::from_str` is used.
rg --type rust 'u32::from_str' -A 5 src/parser/arithmetic.lalrpop
# Step 3: Search for any additional tests related to number parsing.
rg --type rust 'test.*parse.*number'
Length of output: 1235
Tip
Codebase Verification
Ensure the error handling for identifier parsing is robust and tested. Currently, there are no unit tests covering the error message "number is too big" in the arithmetic parser.
- Add unit tests to cover the error handling for the
NumandIdrules insrc/parser/arithmetic.lalrpop.
Analysis chain
Ensure the error handling for identifier parsing is robust and tested.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the error handling for identifier parsing is robust and tested.
# Test: Search for unit tests related to error handling for identifiers. Expect: At least one test.
rg --type rust 'test.*"number is too big"'
Length of output: 44
Script:
#!/bin/bash
# Description: Verify that the error handling for identifier parsing is robust and tested.
# Test: Search for unit tests related to parsing errors in general.
rg --type rust 'test.*parse_error'
# Test: Search for any error messages related to parsing numbers.
rg --type rust 'number is too big'
# Inspect the parser code for error handling mechanisms.
rg --type rust 'from_str' src/parser
Length of output: 223
Script:
#!/bin/bash
# Description: Verify that the error handling for identifier parsing in the arithmetic parser is robust and tested.
# Test: Search for unit tests related to the arithmetic parser.
rg --type rust 'test.*arithmetic'
# Inspect the arithmetic parser code for error handling mechanisms.
rg --type rust 'Result' src/parser/arithmetic.lalrpop
Length of output: 89
Script:
#!/bin/bash
# Description: Verify that the error handling for identifier parsing in the arithmetic parser is robust and tested.
# Test: Search for any other related tests or error handling mechanisms.
rg --type rust 'test.*' src/parser
# Check for comments or documentation related to error handling in the arithmetic parser.
rg --type rust 'error' src/parser/arithmetic.lalrpop
Length of output: 167
Script:
#!/bin/bash
# Description: Verify the context of the error message "number is too big" in the arithmetic parser and search for any indirect references or tests.
# Inspect the specific lines in src/parser/arithmetic.lalrpop where the error message "number is too big" is used.
rg --type rust 'number is too big' src/parser/arithmetic.lalrpop -A 5 -B 5
# Search the entire codebase for any indirect references or tests related to the error message "number is too big".
rg --type rust 'number is too big'
Length of output: 412
README.md
Outdated
| @@ -37,7 +37,7 @@ round_up: true | |||
| less_than_one: ["a * b"] # optional | |||
| greater_than_one: ["c"] # optional | |||
| ``` | |||
| - `formula` contains the formula to be analyze | |||
| - `formula` contains the formula to be analyze. All the numeric literals and identifiers are parsed as positive values, use the `-` sign to include negative values in the formula | |||
There was a problem hiding this comment.
Correct grammatical error in the documentation to enhance clarity.
- formula contains the formula to be analyze.
+ formula contains the formula to be analyzed.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.
| - `formula` contains the formula to be analyze. All the numeric literals and identifiers are parsed as positive values, use the `-` sign to include negative values in the formula | |
| - `formula` contains the formula to be analyze. All the numeric literals and identifiers are parsed as positive values, use the `-` sign to include negative values in the formula | |
| + `formula` contains the formula to be analyzed. All the numeric literals and identifiers are parsed as positive values, use the `-` sign to include negative values in the formula |
| @@ -89,7 +89,7 @@ fn find_less_greater_than_one(expr: &Expr, formula_config: &mut FormulaConfig) { | |||
|
|
|||
| fn visit(expr: &Expr, formula_config: &mut FormulaConfig) { | |||
| match expr { | |||
| Expr::Number(_) | Expr::Id(_) | Expr::Error => (), | |||
| Expr::Number(_) | Expr::Id(_) | Expr::Negative(_) | Expr::Error => (), | |||
There was a problem hiding this comment.
Tip
Codebase Verification
There are no unit tests specifically covering the Expr::Negative variant in the visit function. The search results did not show any test functions referencing Expr::Negative or the visit function in a way that would indicate coverage of this new variant.
- Ensure that unit tests are added to cover the new handling of
Expr::Negativein thevisitfunction.
Analysis chain
Ensure the new handling of Expr::Negative in the visit function is covered by unit tests.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that there are unit tests covering the new handling of `Expr::Negative` in the `visit` function.
# Test: Search for unit tests related to `Expr::Negative`. Expect: At least one test.
rg --type rust 'test.*Expr::Negative'
Length of output: 39
Script:
#!/bin/bash
# Description: Search for the `Expr::Negative` variant in the codebase to locate its usage.
# Search for the definition and usage of `Expr::Negative`.
rg 'Expr::Negative'
# Search for test files that might be testing the `visit` function or `Expr` variants.
rg --type rust 'fn visit'
# Search for any tests that might be indirectly testing `Expr::Negative`.
rg --type rust 'Negative'
Length of output: 2920
Script:
#!/bin/bash
# Description: Search for test functions in the codebase to locate any indirect tests for `Expr::Negative`.
# Search for test functions in the codebase.
rg --type rust '#\[test\]'
# Search for test modules in the codebase.
rg --type rust 'mod test'
Length of output: 930
Script:
#!/bin/bash
# Description: Search within the test functions for references to `Expr::Negative` or the `visit` function.
# Search for references to `Expr::Negative` within test functions.
rg --type rust 'Expr::Negative' src/analyzer.rs src/printer/latex_generator.rs src/analyzer/ast.rs
# Search for references to the `visit` function within test functions.
rg --type rust 'visit' src/analyzer.rs src/printer/latex_generator.rs src/analyzer/ast.rs
Length of output: 878
| fn is_negative_value(expr: Box<Expr>) -> (bool, Box<Expr>) { | ||
| match *expr { | ||
| Expr::Negative(ne) => match *ne { | ||
| Expr::Negative(internal) => is_negative_value(internal), | ||
| _ => (true, ne), | ||
| } | ||
| // Need to handle this case separately because in case of nested negatives the | ||
| // inner most operation will not be simplified | ||
| Expr::Op(l, o, r) => { | ||
| let simplified_internal_expr = simplify_sign(Box::new(Expr::Op(l, o, r))); | ||
|
|
||
| match *simplified_internal_expr { | ||
| Expr::Negative(ne) => (true, ne), | ||
| _ => (false, simplified_internal_expr), | ||
| } | ||
| }, | ||
| _ => (false, expr), | ||
| } | ||
| } |
There was a problem hiding this comment.
Review the logic in is_negative_value for potential simplification or optimization.
Consider simplifying the nested match statements to improve readability and potentially enhance performance.
| fn simplify_sub(lnv: (bool, Box<Expr>), rnv: (bool, Box<Expr>)) -> Box<Expr> { | ||
| let expr = match (lnv.0, rnv.0) { | ||
| (true, false) => Expr::Negative(Box::new(Expr::Op(lnv.1, Opcode::Add, rnv.1))), | ||
| (true, true) => Expr::Op(rnv.1, Opcode::Sub, lnv.1), | ||
| (false, true) => Expr::Op(lnv.1, Opcode::Add, rnv.1), | ||
| (false, false) => Expr::Op(lnv.1, Opcode::Sub, rnv.1), | ||
| }; | ||
|
|
||
| Box::new(expr) | ||
| } |
There was a problem hiding this comment.
Check the logic in simplify_sub for potential errors in handling negative values.
Consider reviewing the handling of negative values to ensure that the function behaves as expected under all conditions.
| fn simplify_add(lnv: (bool, Box<Expr>), rnv: (bool, Box<Expr>)) -> Box<Expr> { | ||
| let expr = match (lnv.0, rnv.0) { | ||
| (true, false) => Expr::Op(rnv.1, Opcode::Sub, lnv.1), | ||
| (true, true) => Expr::Negative(Box::new(Expr::Op(lnv.1, Opcode::Add, rnv.1))), | ||
| (false, true) => Expr::Op(lnv.1, Opcode::Sub, rnv.1), | ||
| (false, false) => Expr::Op(lnv.1, Opcode::Add, rnv.1), | ||
| }; | ||
|
|
||
| Box::new(expr) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The simplify_add function is referenced within the match statement handling different Opcode variants. However, there are no direct or indirect tests found for simplify_add in the test files.
- Please add unit tests for the
simplify_addfunction to ensure its correctness.
Analysis chain
Validate the correctness of the simplify_add function and ensure it is covered by unit tests.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that there are unit tests covering the `simplify_add` function.
# Test: Search for unit tests related to `simplify_add`. Expect: At least one test.
rg --type rust 'test.*simplify_add'
Length of output: 37
Script:
#!/bin/bash
# Locate the definition of the `simplify_add` function to confirm its location.
ast-grep --lang rust --pattern 'fn simplify_add($_, $_) -> $_ { $$$ }'
# Search for any references to `simplify_add` in the codebase to identify potential indirect tests.
rg 'simplify_add'
Length of output: 1054
Script:
#!/bin/bash
# Locate the context in which `simplify_add` is referenced within `src/analyzer/simplify_expr.rs`.
rg 'simplify_add' src/analyzer/simplify_expr.rs -A 5 -B 5
# Search for any test files or modules that might contain tests for `simplify_add`.
rg 'simplify_add' --type rust --glob '*test*'
Length of output: 1001
| fn visit(expr: &Expr, rounding_direction: bool, formula_config: &mut FormulaConfig) -> Result<()> { | ||
| match expr { | ||
| Expr::Number(_) | Expr::Id(_) | Expr::Error => (), | ||
| Expr::Negative(nexpr) => visit(nexpr, !rounding_direction, formula_config)?, |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate that there are no direct unit tests for Expr::Negative in the test files. This suggests that the new handling of Expr::Negative in the visit function is not explicitly covered by unit tests.
- Location of
visitfunction:src/analyzer/analyze_rounding.rsat line 79. - References to
Expr::Negative: Found in multiple files, but no direct unit tests.
To ensure the new handling of Expr::Negative in the visit function is properly tested, consider adding unit tests that specifically cover this case.
- Files to update:
- Add unit tests in the relevant test files to cover
Expr::Negative.
- Add unit tests in the relevant test files to cover
Analysis chain
Ensure the new handling of Expr::Negative in the visit function is covered by unit tests.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that there are unit tests covering the new handling of `Expr::Negative` in the `visit` function.
# Test: Search for unit tests related to `Expr::Negative`. Expect: At least one test.
rg --type rust 'test.*Expr::Negative'
Length of output: 39
Script:
#!/bin/bash
# Description: Verify the presence of unit tests covering the new handling of `Expr::Negative` in the `visit` function.
# Step 1: Locate the definition of the `visit` function.
ast-grep --lang rust --pattern 'fn visit($_, $_, $_) -> $_ { $$$ }'
# Step 2: Search for references to `Expr::Negative` in test files.
rg --type rust 'Expr::Negative'
Length of output: 2840
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- README.md (1 hunks)
- src/analyzer.rs (2 hunks)
- src/analyzer/simplify_expr.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/analyzer/simplify_expr.rs
Additional context used
LanguageTool
README.md
[uncategorized] ~14-~14: Possible missing article found.
Context: ...direction of the substracted element is inverse of the expected rounding) - `A * B => r...
[style] ~25-~25: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...to generate user configuration file. - Runroundme analyzeto analyze the config...
[grammar] ~40-~40: The past participle is required after “to be”, alternatively you could omit the “be”.
Context: ...``` -formulacontains the formula to be analyze. All the numeric literals and identifie...
Markdownlint
README.md
23-23: Expected: 0 or 2; Actual: 1
Trailing spaces
24-24: Expected: 0 or 2; Actual: 1
Trailing spaces
5-5: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
34-34: null
Fenced code blocks should be surrounded by blank lines
39-39: null
Fenced code blocks should be surrounded by blank lines
50-50: null
Fenced code blocks should be surrounded by blank lines
55-55: null
Fenced code blocks should be surrounded by blank lines
6-6: null
Lists should be surrounded by blank lines
40-40: null
Lists should be surrounded by blank lines
15-15: null
Spaces inside code span elements
Additional comments not posted (2)
src/analyzer.rs (2)
2-2: The addition of thesimplify_exprmodule is well-integrated.
23-23: The refactoredanalyzefunction correctly simplifies the AST before further processing.
README.md
Outdated
| @@ -37,7 +37,7 @@ round_up: true | |||
| less_than_one: ["a * b"] # optional | |||
| greater_than_one: ["c"] # optional | |||
| ``` | |||
| - `formula` contains the formula to be analyze | |||
| - `formula` contains the formula to be analyze. All the numeric literals and identifiers are parsed as positive values, use the `-` sign to include negative values in the formula. The negative sign of the base in the exponential operations are ignored. | |||
There was a problem hiding this comment.
Correct grammatical error in the documentation to enhance clarity.
- formula contains the formula to be analyze.
+ formula contains the formula to be analyzed.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.
| - `formula` contains the formula to be analyze. All the numeric literals and identifiers are parsed as positive values, use the `-` sign to include negative values in the formula. The negative sign of the base in the exponential operations are ignored. | |
| - `formula` contains the formula to be analyzed. All the numeric literals and identifiers are parsed as positive values, use the `-` sign to include negative values in the formula. The negative sign of the base in the exponential operations are ignored. |
src/analyzer.rs
Outdated
| println!("parsed : {ast}"); | ||
| let simplified_ast = simplify_expr::simplify_sign(ast); | ||
| println!("simplified: {simplified_ast}"); |
There was a problem hiding this comment.
Adding debug print statements can be useful for development but should be removed or replaced with proper logging before merging to production.
- println!("parsed : {ast}");
- println!("simplified: {simplified_ast}");
+ // Consider replacing with a logging framework
+ // log::debug!("parsed : {ast}");
+ // log::debug!("simplified: {simplified_ast}");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.
| println!("parsed : {ast}"); | |
| let simplified_ast = simplify_expr::simplify_sign(ast); | |
| println!("simplified: {simplified_ast}"); | |
| // Consider replacing with a logging framework | |
| // log::debug!("parsed : {ast}"); | |
| // log::debug!("simplified: {simplified_ast}"); | |
| let simplified_ast = simplify_expr::simplify_sign(ast); |
| @@ -13,7 +14,148 @@ pub fn analyze(formula_config: &mut FormulaConfig) -> anyhow::Result<Box<Expr>> | |||
| anyhow::anyhow!("Error occured while parsing the formula {}: {}", formula, e) | |||
| })?; | |||
|
|
|||
| analyze_rounding::analyze(&ast, formula_config.round_up, formula_config)?; | |||
| println!("parsed : {ast}"); | |||
| let simplified_ast = simplify_expr::simplify_sign(ast); | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure that the simplify_sign function is thoroughly tested, especially since it manipulates the AST directly.
- The
simplify_signfunction is not directly tested in the codebase. - The test module in
src/analyzer.rsdoes not contain tests forsimplify_sign.
Analysis chain
Ensure that the simplify_sign function is thoroughly tested, especially since it manipulates the AST directly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the presence of tests for `simplify_sign`.
# Test: Search for the function usage in tests. Expect: At least one occurrence.
rg --type rust $'simplify_sign' ./src/analyzer/tests.rs
Length of output: 118
Script:
#!/bin/bash
# Description: Verify the presence of tests for `simplify_sign`.
# Test: Search for the function usage in all test files. Expect: At least one occurrence.
rg --type rust 'simplify_sign' --glob '*test*'
Length of output: 46
Script:
#!/bin/bash
# Description: Verify the presence of tests for `simplify_sign`.
# Step 1: Find the definition of `simplify_sign`.
ast-grep --lang rust --pattern 'fn simplify_sign($_) { $$$ }'
# Step 2: Search for the usage of `simplify_sign` across the repository.
rg 'simplify_sign'
Length of output: 525
Script:
#!/bin/bash
# Description: Verify the presence of tests for `simplify_sign`.
# Step 1: Find test files related to `src/analyzer/simplify_expr.rs` and `src/analyzer.rs`.
test_files=$(fd --type f --glob '*test*.rs')
# Step 2: Search for tests involving `simplify_sign` in the identified test files.
for test_file in $test_files; do
echo "Searching in $test_file"
rg 'simplify_sign' "$test_file"
done
Length of output: 47
Script:
#!/bin/bash
# Description: Verify the presence of tests for `simplify_sign`.
# Step 1: Search for test modules within `src/analyzer/simplify_expr.rs` and `src/analyzer.rs`.
rg '#\[cfg\(test\)\]' src/analyzer/simplify_expr.rs src/analyzer.rs
Length of output: 98
Script:
#!/bin/bash
# Description: Verify the presence of tests for `simplify_sign`.
# Step 1: Search for the usage of `simplify_sign` within the test module in `src/analyzer.rs`.
rg 'simplify_sign' src/analyzer.rs --context 10
Length of output: 718
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
README.md (2)
Line range hint
23-24: Remove trailing spaces to comply with markdown best practices.- Run `roundme init` to generate user configuration file. - Run `roundme analyze` to analyze the configuration file + Run `roundme init` to generate user configuration file. + Run `roundme analyze` to analyze the configuration file.
Line range hint
5-6: Ensure proper markdown formatting by adding necessary blank lines around headings, lists, and fenced code blocks.+ ## Features - Recommends whether an arithmetic operation needs to round up or down + + ```yaml formula: a * b / c round_up: true less_than_one: ["a * b"] # optional greater_than_one: ["c"] # optional + + ```bash cargo install roundme + +Also applies to: 34-34, 39-39, 50-50, 55-55
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- README.md (1 hunks)
- src/analyzer.rs (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/analyzer.rs
Additional context used
LanguageTool
README.md
[uncategorized] ~14-~14: Possible missing article found.
Context: ...direction of the substracted element is inverse of the expected rounding) - `A * B => r...
[style] ~25-~25: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...to generate user configuration file. - Runroundme analyzeto analyze the config...
Markdownlint
README.md
23-23: Expected: 0 or 2; Actual: 1
Trailing spaces
24-24: Expected: 0 or 2; Actual: 1
Trailing spaces
5-5: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
34-34: null
Fenced code blocks should be surrounded by blank lines
39-39: null
Fenced code blocks should be surrounded by blank lines
50-50: null
Fenced code blocks should be surrounded by blank lines
55-55: null
Fenced code blocks should be surrounded by blank lines
6-6: null
Lists should be surrounded by blank lines
40-40: null
Lists should be surrounded by blank lines
15-15: null
Spaces inside code span elements
Additional comments not posted (1)
README.md (1)
40-40: Clarification on handling negative values in formulas aligns well with the PR objectives.
Summary by CodeRabbit
New Features
FormulaConfigwith specific parameters.Bug Fixes
Documentation
formulafield in the README.Tests
Enhancements