Fix/dict key access #19
Conversation
WalkthroughThe pull request introduces significant changes to 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## main #19 +/- ##
=======================================
Coverage 97.86% 97.86%
=======================================
Files 24 24
Lines 7822 7840 +18
=======================================
+ Hits 7655 7673 +18
Misses 134 134
Partials 33 33
🚨 Try these New Features:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
engine/text_test.go (2)
364-377: Consider adding bytecode sequence documentation.The bytecode sequence is correct but could benefit from a comment explaining the operation flow:
- Variable setup
- Predicate call
- Dictionary field access
- Unification
- Exit
Add this comment before the bytecode:
bytecode: bytecode{ + // Bytecode sequence: + // 1. Set up variables + // 2. Call p(P) to get the dictionary + // 3. Access P.x using the dot operator + // 4. Unify X with the field value {opcode: OpGetVar, operand: Integer(0)}, // ... rest of the bytecode
331-382: Consider adding test cases for edge cases.While the current test coverage for dictionary operations is good, consider adding test cases for:
- Nested dictionary access (e.g.,
P.x.y)- Non-existent field access
- Invalid field access (e.g., accessing a field of a non-dictionary)
Example test case for nested access:
+ {title: "dict body (nested)", text: ` +x(X) :- p(P), =(X, P.x.y). +`, result: buildOrderedMap( + // ... appropriate result map + )},interpreter_test.go (1)
1058-1064: LGTM! Consider adding more test cases.The test case correctly verifies the dictionary key access functionality through predicate chaining. This aligns well with the PR's objective of fixing dict key access issues.
Consider adding more test cases to cover:
- Accessing non-existent keys through predicate chains
- Accessing nested dictionaries through longer predicate chains
- Error handling when the dictionary is not properly initialized in the predicate chain
engine/clause.go (2)
Line range hint
118-149: Consider adding unit tests fordesugarPredfunctionGiven the complexity of the
desugarPredfunction, adding unit tests would help ensure its correctness across various term types and improve code reliability.
Line range hint
118-149: Enhance code readability with detailed comments indesugarPredAdding inline comments to explain the recursive desugaring logic in
desugarPredcan improve maintainability and assist future developers in understanding the code flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 7be6699 and fe6edb60679f705ab88fd24b537a7ca70bdaded8.
📒 Files selected for processing (3)
engine/clause.go(3 hunks)engine/text_test.go(1 hunks)interpreter_test.go(1 hunks)
🔇 Additional comments (5)
engine/text_test.go (1)
331-382: LGTM! Well-structured test case for dictionary field access.
The test case effectively verifies the compilation and bytecode generation for dictionary field access in rule bodies. It complements existing dictionary-related tests by covering the dot notation access pattern.
engine/clause.go (4)
67-68: Simplify clause compilation by separating desugaring of head and body
The use of desugarHead and desugarBody functions enhances modularity and clarity in the compileClause function.
70-76: Ensure correct sequencing of desugared predicates with body
The logic correctly appends the desugared predicates from the head to the body of the clause, preserving the correct execution order.
93-98: Correct handling of head term in desugarHead function
The function safely handles both compound and non-compound head terms, ensuring that non-compound terms are returned unchanged.
100-116: Efficient desugaring of body terms
The desugarBody function correctly iterates over the body terms, desugaring each one and collecting any additional predicates.
ccamel
force-pushed
the
fix/dict-key-access
branch
from
fe6edb6 to
21ef28c
Compare
ccamel
force-pushed
the
fix/dict-key-access
branch
from
21ef28c to
b1bd213
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
engine/text_test.go (1)
383-434: LGTM! Consider adding edge cases for nested dict access.The test case effectively verifies dict access in clause body with variable binding. To make it even more robust, consider adding test cases for nested dict access patterns (e.g.,
P.x.y).Example test case to consider:
{title: "dict body (2)", text: ` x(X) :- p(P), =(X, P.x). +y(Y) :- p(P), =(Y, P.x.y). `, result: buildOrderedMap(interpreter_test.go (1)
1079-1085: Consider adding error cases for compound queriesWhile the test case verifies successful compound queries with dict key access, consider adding test cases for error conditions in compound queries, such as:
- Accessing non-existent keys in compound queries
- Type errors in compound queries
- Invalid key access patterns
Example test case to add:
+ { + program: "p(P) :- P = point{x:10}. x(X) :- p(P), X = P.z.", + query: "x(X).", + wantResult: []result{{ + err: fmt.Errorf("error(domain_error(dict_key,z),. /3)"), + }}, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between fe6edb60679f705ab88fd24b537a7ca70bdaded8 and b1bd213.
📒 Files selected for processing (2)
engine/text_test.go(2 hunks)interpreter_test.go(2 hunks)
🔇 Additional comments (4)
engine/text_test.go (1)
285-336: LGTM! Well-structured test case for dict access in clause head.
The test case thoroughly verifies the compilation of dict access in clause head with a body, ensuring correct bytecode generation for both dict construction and field access operations.
interpreter_test.go (3)
1025-1031: LGTM: Test case verifies dict key access in rule body
The test case correctly verifies that dictionary key access works within a rule body, ensuring that the ok predicate is properly called before accessing the dict key.
1033-1038: LGTM: Test case verifies variable binding with dict key access
The test case properly verifies that variables can be bound when accessing dictionary keys, with the variable X being bound to 5 in the rule body.
1040-1045: LGTM: Test case verifies variable key access
The test case correctly verifies that dictionary keys can be accessed using variables, with the variable X being bound to the key name x.
This PR fixes the ordering of desugared terms for Dicts in clause bodies (introduced by #17) and adds tests to verify the corrected behavior.
Additionally, it includes a minor refactor of the desugaring process by separating the logic for head and body terms into distinct functions.