367 tags come after link would be parse #382
Conversation
Signed-off-by: Kilerd Chan <[email protected]>
Signed-off-by: Kilerd Chan <[email protected]>
Signed-off-by: Kilerd Chan <[email protected]>
…e CI Signed-off-by: Kilerd Chan <[email protected]>
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
WalkthroughThe pull request includes modifications to several files aimed at improving the handling of tags and links in financial transactions and enhancing the CI workflow. Changes in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
integration-tests/random-order-of-links-and-tags/validations.json (2)
5-28: LGTM: Consistent validations for first two recordsThe validations for records[0] and records[1] are correctly structured and consistent. Both records are expected to have a single link ("link1") and a single tag ("tag1").
Consider if this repetition is intentional. If these two records always have identical expectations, you might optimize the JSON structure by grouping them. For example:
[ { "paths": ["$.data.records[0]", "$.data.records[1]"], "expectations": { "links": ["link1"], "tags": ["tag1"] } } ]This could make the validation file more concise and easier to maintain.
29-84: LGTM: Consistent validations for records[2] to records[5]The validations for records[2] through records[5] are correctly structured and consistent. Each record is expected to have two links ("link1", "link2") and two tags ("tag1", "tag2").
To reduce repetition and improve maintainability, consider optimizing this section. You could use a single validation rule for all four records. For example:
[ { "paths": ["$.data.records[2]", "$.data.records[3]", "$.data.records[4]", "$.data.records[5]"], "expectations": { "links": ["link1", "link2"], "tags": ["tag1", "tag2"] } } ]This approach would make the validation file more concise and easier to update if the expectations change in the future.
extensions/beancount/src/beancount.pest (1)
40-43: Newtags_or_linksrule enhances parsing flexibilityThe introduction of the
tags_or_linksrule is a good solution for handling tags and links more flexibly. This change allows for any combination and order of tags and links within a transaction, which is a significant improvement in the grammar's expressiveness.Consider a minor optimization to the rule:
-tags_or_links = { (space* ~ (tag | link))* } +tags_or_links = { (space+ ~ (tag | link))+ }This change ensures at least one tag or link is present and requires a space before each, which may better reflect typical Beancount syntax. However, if you need to allow for no tags or links, or tags/links without preceding spaces, the current implementation is correct.
zhang-cli/src/main.rs (3)
269-274: LGTM! Consider using a constant for main file names.The new loop for detecting main files (
main.zhangandmain.bean) improves flexibility and maintainability. This approach allows easy addition of new file types in the future.Consider defining the main file names as a constant array at the module level for better maintainability:
const MAIN_FILE_NAMES: [&str; 2] = ["main.zhang", "main.bean"]; // Then in the function: for main_file in MAIN_FILE_NAMES { // ... rest of the code }
275-292: LGTM! Consider improving error handling for ledger loading.The creation of OpendalDataSource and Ledger for each detected main file is well-implemented. The use of async/await for ledger loading is appropriate.
Consider improving the error handling for ledger loading. Instead of using
expect, which may terminate the test abruptly, you could useunwrap_or_elseto provide more context:let ledger = Ledger::async_load(test_temp_folder.to_path_buf(), main_file.to_string(), data_source.clone()) .await .unwrap_or_else(|e| panic!("Failed to load ledger for {}: {}", main_file, e));This approach will provide more informative error messages if ledger loading fails.
298-338: LGTM! Consider extracting validation logic to a separate function.The validation logic is thorough and well-implemented. The use of JSON Path for validation is flexible and allows for complex checks. The error messages are detailed, which is excellent for debugging failed tests.
To improve readability and maintainability, consider extracting the validation logic into a separate function:
fn validate_response(res: Value, validations: &[ValidationPoint], test_case: &str, uri: &str) { for point in validations { // ... (existing validation logic) } } // In the main test function: validate_response(res, &validation.validations, &original_test_source_folder.display().to_string(), &validation.uri);This refactoring would make the main test function more concise and easier to read, while also making the validation logic reusable if needed.
zhang-core/src/data_type/text/zhang.pest (1)
28-28: Consider adding test cases for the new tags and links orderingNow that the grammar permits tags and links in any order, it's advisable to add or update unit tests to cover various combinations. This ensures that the parser correctly handles all possible scenarios and maintains robustness.
Would you like assistance in generating test cases to validate these changes?
extensions/beancount/src/parser.rs (1)
Line range hint
384-392: Handle potential cases of bothPostingandMetabeing presentIn the loop processing
ret.5, the match statement does not handle cases where bothPostingandMetaareSome. This could lead to missing data if a transaction line contains both a posting and metadata.Consider updating the match statement to handle this case explicitly:
for line in ret.5 { match line { (Some(trx), None) => { transaction.postings.push(trx); } (None, Some(meta)) => { transaction.meta.insert(meta.0, meta.1); + } + (Some(trx), Some(meta)) => { + transaction.postings.push(trx); + transaction.meta.insert(meta.0, meta.1); } - _ => {} + (None, None) => { + // Handle or log unexpected empty lines if necessary + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- .github/workflows/build-latest.yml (0 hunks)
- extensions/beancount/src/beancount.pest (1 hunks)
- extensions/beancount/src/parser.rs (2 hunks)
- integration-tests/random-order-of-links-and-tags/main.bean (1 hunks)
- integration-tests/random-order-of-links-and-tags/main.zhang (1 hunks)
- integration-tests/random-order-of-links-and-tags/validations.json (1 hunks)
- zhang-cli/src/main.rs (1 hunks)
- zhang-core/src/data_type/text/parser.rs (2 hunks)
- zhang-core/src/data_type/text/zhang.pest (1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/build-latest.yml
🧰 Additional context used
🔇 Additional comments (16)
integration-tests/random-order-of-links-and-tags/main.bean (4)
1-1: LGTM: Currency setup is correct.The operating currency is properly set to CNY (Chinese Yuan) using the "option" directive.
3-5: LGTM: Account openings are correct and consistent.The Assets:BankCard and Expenses:Food accounts are properly opened with CNY as the currency, consistent with the operating currency. The use of 1970-01-01 as the opening date is a common practice for account initialization in Beancount.
7-27: LGTM: Varied tag and link ordering aligns with PR objective.The intentional variation in the order of tags (#tag1, #tag2) and links (^link1, ^link2) across transactions appears to be designed to test parsing capabilities. This aligns well with the PR title "367 tags come after link would be parse".
The consistent use of the same tags and links across transactions, but in different orders, provides a good test suite for ensuring that the parser can handle various combinations correctly.
7-29: LGTM: Transaction structure is consistent, but note the repetitive nature.The transactions are structured consistently, with proper date, payee, narration, and account postings. All transactions deduct 50 CNY from Assets:BankCard and credit Expenses:Food.
Note: The repetitive nature of these transactions (same amount, payee, and description) suggests this file might be used for testing purposes, particularly for tag and link ordering.
To confirm the consistency of the transactions, run the following script:
The output of both commands should be 6, matching the number of transactions in the file.
✅ Verification successful
Verification Successful: All transactions are consistent.
All transactions have matching amounts and descriptions as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency of transaction amounts and descriptions # Test: Check if all transactions have the same amount and description rg --type-add 'bean:*.bean' --type bean '"KFC" "VME50 Package"' | wc -l rg --type-add 'bean:*.bean' --type bean 'Assets:BankCard -50 CNY' | wc -lLength of output: 153
integration-tests/random-order-of-links-and-tags/main.zhang (3)
1-5: LGTM: Currency and account setup is correct.The operating currency is properly set to CNY, and the necessary accounts (Assets:BankCard and Expenses:Food) are opened with the correct currency. Using 1970-01-01 as the opening date is a common practice in financial systems.
7-29: LGTM: Transactions are correctly structured with intentional variations.The six transactions from 2023-12-01 to 2023-12-06 are consistently structured:
- Each is for "KFC" "VME50 Package" with a value of 50 CNY.
- Correct debiting from Assets:BankCard and crediting to Expenses:Food.
- Varying combinations and orders of tags (#tag1, #tag2) and links (^link1, ^link2).
The intentional variation in tag and link order appears to be for testing parsing capabilities, which aligns with the PR objective of addressing tag parsing after links.
1-29: File effectively serves as a test case for tag and link parsing.This file successfully sets up a test environment for validating improvements in parsing tags and links in financial transactions. It includes:
- Proper currency and account setup.
- Multiple transactions with intentionally varied combinations and orders of tags and links.
These variations provide an excellent test case for ensuring that the parser can handle different arrangements of tags and links, directly addressing the PR's objective of improving tag parsing after links.
integration-tests/random-order-of-links-and-tags/validations.json (2)
1-4: LGTM: Well-structured JSON for API validationsThe overall structure of this JSON file is well-organized and clearly defines the purpose of the validations. It correctly specifies the API endpoint ("/api/journals") and provides an array for individual validation rules.
Also applies to: 85-87
1-87: Overall: Well-structured but with room for optimizationThis validation file successfully defines the expected structure and content for the "/api/journals" API response. It correctly validates the presence and values of links and tags across multiple records.
To enhance maintainability and reduce redundancy, consider the following optimizations:
- Group identical validations for records[0] and records[1].
- Use a single validation rule for records[2] through records[5].
These changes would make the file more concise and easier to update in the future, while maintaining the same validation coverage.
To ensure these validations align with the actual API implementation, you may want to run the following check:
This script will help confirm that the API implementation aligns with these validation expectations.
extensions/beancount/src/beancount.pest (2)
36-37: Improved flexibility in parsing tags and linksThe modification to the
transactionrule enhances the grammar's ability to handle tags and links more flexibly. By replacingtags? ~ links?withtags_or_links?, the parser can now accept tags and links in any order within a transaction, addressing the issue mentioned in the PR title.This change improves the robustness of the parser while maintaining backward compatibility with existing Beancount files.
36-43: Summary: Improved tag and link parsing in transactionsThe changes to the
transactionrule and the introduction of thetags_or_linksrule effectively address the issue of parsing tags that come after links. These modifications improve the flexibility of the Beancount grammar while maintaining backward compatibility with existing files.To ensure the changes work as expected, consider running the following verification:
This script helps identify relevant test files. Ensure that your test suite includes cases that cover various combinations of tags and links, including scenarios where tags come after links.
zhang-cli/src/main.rs (1)
269-338: Overall, excellent improvements to the integration test.The changes to the
integration_testfunction significantly enhance its flexibility and robustness. The new implementation supports multiple main file types, provides detailed error messages, and uses a more maintainable approach for file detection and validation.Key improvements:
- Support for multiple main file types (
main.zhangandmain.bean).- Flexible and reusable validation logic using JSON Path.
- Detailed error messages for failed tests.
The suggested minor improvements (using constants, improving error handling, and extracting validation logic) would further enhance the code's maintainability and readability. Great work on improving the testing process!
zhang-core/src/data_type/text/zhang.pest (2)
28-28: Consolidation of tags and links intotags_or_linksis appropriateThe modification to use
tags_or_links?in thetransactionrule simplifies the grammar and allows tags and links to appear in any order within a transaction. This change effectively addresses the issue where tags following links were not being parsed correctly.
37-40: The newtags_or_linksrule effectively combines tags and linksIntroducing the
tags_or_linksrule with the definition{ (space* ~ (tag | link))* }appropriately merges the previoustagsandlinksrules. This allows for a flexible ordering of tags and links, enhancing the grammar's expressiveness.extensions/beancount/src/parser.rs (1)
367-371: Verify all transaction parsing patterns are updatedWith the consolidation of tags and links into
tags_or_links, ensure that all transaction patterns in thematch_nodes!macro are correctly updated to reflect the new structure. This helps prevent parsing errors for different transaction syntaxes.To verify, consider reviewing all transaction test cases to confirm they are parsed correctly.
zhang-core/src/data_type/text/parser.rs (1)
377-381: Transaction patterns correctly updated to usetags_or_linksThe transaction parsing patterns have been appropriately updated to incorporate the
tags_or_linksmethod. The changes ensure that both tags and links are parsed together, simplifying the logic.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Kilerd Chan <[email protected]>
Kilerd
force-pushed
the
367-tags-come-after-link-would-be-parse
branch
from
e8494bc to
59d8348
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor