Refactor/substitution expression #555
Conversation
WalkthroughThe recent modifications primarily revolve around the transition from using Changes
Related issues
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 (
|
ccamel
force-pushed
the
refactor/substituion-expression
branch
from
a75b438 to
5a3c040
Compare
ccamel
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
x/logic/types/types.pb.gois excluded by:!**/*.pb.go
Files selected for processing (16)
- docs/proto/logic.md (2 hunks)
- proto/logic/v1beta2/types.proto (1 hunks)
- x/logic/keeper/grpc_query_ask_test.go (7 hunks)
- x/logic/keeper/interpreter.go (6 hunks)
- x/logic/predicate/address_test.go (4 hunks)
- x/logic/predicate/bank_test.go (26 hunks)
- x/logic/predicate/builtin_test.go (2 hunks)
- x/logic/predicate/crypto_test.go (12 hunks)
- x/logic/predicate/did_test.go (2 hunks)
- x/logic/predicate/encoding_test.go (2 hunks)
- x/logic/predicate/file_test.go (11 hunks)
- x/logic/predicate/json_test.go (17 hunks)
- x/logic/predicate/string_test.go (13 hunks)
- x/logic/predicate/uri_test.go (4 hunks)
- x/logic/testutil/logic.go (1 hunks)
- x/logic/wasm/types.go (3 hunks)
Additional comments: 69
proto/logic/v1beta2/types.proto (1)
- 15-16: LGTM!
x/logic/predicate/builtin_test.go (1)
- 61-61: LGTM!
x/logic/testutil/logic.go (1)
- 14-15: LGTM!
x/logic/wasm/types.go (3)
- 16-19: LGTM!
- 38-38: LGTM!
- 78-79: LGTM!
x/logic/predicate/encoding_test.go (2)
- 30-30: LGTM!
- 110-110: LGTM!
x/logic/predicate/did_test.go (1)
- 30-30: LGTM!
x/logic/predicate/address_test.go (1)
- 30-30: LGTM!
x/logic/keeper/grpc_query_ask_test.go (10)
- 35-43: LGTM!
- 53-54: LGTM!
- 66-67: LGTM!
- 79-80: LGTM!
- 102-103: LGTM!
- 115-116: LGTM!
- 128-129: LGTM!
- 141-142: LGTM!
- 157-158: LGTM!
- 172-172: LGTM!
x/logic/keeper/interpreter.go (6)
- 6-6:
stringsimport added but not directly used in the provided code changes.- 9-9:
github.com/ichiban/prolog/engineimport added and correctly used in thequeryInterpreterfunction.- 44-50: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [47-57]
In
execute, correctly refactored to usequeryInterpreter. Ensurelimits.MaxResultCountis properly checked before dereferencing.
- 75-109:
queryInterpreterfunction correctly handles query execution and result processing. However, ensure error handling and limit checks are robust and tested.- 164-172:
checkLimitsfunction correctly checks query size limits against provided limits. Ensure error messages are clear and tested.- 198-223: Utility functions
parsedVarsToVarsandenvsToResultscorrectly process variables and results. Ensure error handling inenvsToResultsis robust.x/logic/predicate/uri_test.go (1)
- 39-144: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [30-160]
Replaced
types.TermResultswithtestutil.TermResultsin test cases, aligning with the new substitution mechanism. Ensure all test cases are updated and correctly usetestutil.TermResults.x/logic/predicate/string_test.go (1)
- 28-42: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [31-360]
Replaced
types.TermResultswithtestutil.TermResultsin test cases, aligning with the new substitution mechanism. Ensure all test cases are updated and correctly usetestutil.TermResults.x/logic/predicate/file_test.go (14)
- 39-39: Ensure the replacement of
types.TermResultswithtestutil.TermResultsaligns with the new substitution mechanism and does not alter the intended test outcomes.- 51-51: Confirm that the empty
testutil.TermResultscorrectly represents the expected outcome for this test case, given the changes in substitution representation.- 57-57: Verify that the empty
testutil.TermResultsaccurately reflects the expected result, considering the updated substitution mechanism.- 73-73: Check if the representation of
testutil.TermResultswith multiple entries correctly matches the expected test outcomes after the substitution mechanism update.- 79-79: Ensure the representation of
testutil.TermResultswith multiple entries is consistent with the expected outcomes, given the changes to the substitution mechanism.- 85-85: Confirm that the empty
testutil.TermResultsand the specifiedwantErrorare appropriate for the test case, considering the substitution mechanism changes.- 97-97: Verify that the empty
testutil.TermResultscorrectly represents the expected outcome for this test case, in light of the substitution mechanism changes.- 113-113: Check if the representation of
testutil.TermResultswith a single entry accurately reflects the expected test outcomes after the substitution mechanism update.- 119-119: Ensure the representation of
testutil.TermResultswith a single entry is consistent with the expected outcomes, given the changes to the substitution mechanism.- 153-155: Review the loop for scanning solutions to ensure it correctly handles the conversion of solutions to
testutil.TermResultsand properly checks for errors and expected results.- 200-200: Confirm the replacement of
types.TermResultswithtestutil.TermResultsaligns with the new substitution mechanism and does not alter the intended test outcomes.- 210-210: Verify that the
testutil.TermResultswith a single entry correctly represents the expected outcome for this test case, considering the substitution mechanism changes.- 221-221: Check if the representation of
testutil.TermResultswith a single entry accurately reflects the expected test outcomes after the substitution mechanism update.- 346-348: Review the loop for scanning solutions to ensure it correctly handles the conversion of solutions to
testutil.TermResultsand properly checks for errors and expected results.x/logic/predicate/crypto_test.go (19)
- 30-30: Ensure the replacement of
types.TermResultswithtestutil.TermResultsaligns with the new substitution mechanism and does not alter the intended test outcomes.- 37-37: Confirm that the
testutil.TermResultswith a single entry correctly represents the expected outcome for this test case, considering the substitution mechanism changes.- 45-45: Verify that the empty
testutil.TermResultscorrectly represents the expected outcome for this test case, in light of the substitution mechanism changes.- 51-51: Check if the representation of
testutil.TermResultswith a single entry accurately reflects the expected test outcomes after the substitution mechanism update.- 59-59: Ensure the representation of
testutil.TermResultswith a single entry is consistent with the expected outcomes, given the changes to the substitution mechanism.- 67-67: Confirm that the
testutil.TermResultswith a single entry correctly represents the expected outcome for this test case, considering the substitution mechanism changes.- 75-75: Verify that the
testutil.TermResultswith a single entry accurately reflects the expected test outcomes after the substitution mechanism update.- 88-88: Check if the representation of
testutil.TermResultswith a single entry is appropriate for the expected outcomes, given the changes to the substitution mechanism.- 96-96: Ensure the representation of
testutil.TermResultswith a single entry is consistent with the expected outcomes, considering the substitution mechanism changes.- 125-127: Review the loop for scanning solutions to ensure it correctly handles the conversion of solutions to
testutil.TermResultsand properly checks for errors and expected results.- 166-166: Confirm the replacement of
types.TermResultswithtestutil.TermResultsaligns with the new substitution mechanism and does not alter the intended test outcomes.- 178-178: Verify that the empty
testutil.TermResultscorrectly represents the expected outcome for this test case, in light of the substitution mechanism changes.- 187-187: Check if the empty
testutil.TermResultsaccurately reflects the expected test outcomes after the substitution mechanism update.- 248-248: Ensure the representation of
testutil.TermResultswith a single entry is consistent with the expected outcomes, given the changes to the substitution mechanism.- 280-280: Verify that the empty
testutil.TermResultscorrectly represents the expected outcome for this test case, considering the substitution mechanism changes.- 291-291: Check if the empty
testutil.TermResultsaccurately reflects the expected test outcomes after the substitution mechanism update.- 303-303: Confirm that the
testutil.TermResultswith a single entry correctly represents the expected outcome for this test case, in light of the substitution mechanism changes.- 314-314: Ensure the representation of
testutil.TermResultswith a single entry is consistent with the expected outcomes, considering the substitution mechanism changes.- 342-344: Review the loop for scanning solutions to ensure it correctly handles the conversion of solutions to
testutil.TermResultsand properly checks for errors and expected results.x/logic/predicate/json_test.go (5)
- 31-31: Ensure the replacement of
types.TermResultswithtestutil.TermResultsis consistent across all test cases. This change aligns with the PR objectives to update the test files for the new substitution representation.- 53-53: The test cases correctly use
testutil.TermResultsfor expected results, aligning with the PR's objective to refactor the substitution mechanism. Each test case is well-described, and the expected outcomes are clearly defined.Also applies to: 61-61, 71-71, 79-79, 87-87, 95-95, 105-105, 129-129, 137-137, 147-147, 157-157, 165-165, 173-173, 181-181, 192-192, 200-200, 208-208, 216-216, 224-224, 234-234, 242-242, 250-250, 258-258, 286-286, 302-302, 310-310, 318-318, 326-326, 342-342, 350-350, 360-360
- 388-388: The loop for processing solution bindings correctly initializes
testutil.TermResultsfor each iteration. This is consistent with the updated substitution representation.- 390-390: The call to
sols.Scan(m)correctly passes thetestutil.TermResultsinstance. This ensures that the test cases are aligned with the new substitution mechanism.- 492-492: The handling of solution bindings in the more complex struct bidirectional tests is consistent with the changes made in the simpler test cases. The usage of
testutil.TermResultsand the scanning process align with the PR's objectives.Also applies to: 494-494, 531-531, 533-533
x/logic/predicate/bank_test.go (2)
- 40-40: The change from
types.TermResultstotestutil.TermResultsaligns with the PR objectives to update the test files for the new substitution representation.- 486-486: The call to
sols.Scan(m)directly passes a map (testutil.TermResults), assuming it's correctly handled by the method. Ensure thatsols.Scan()is implemented to accept and correctly populate a map structure.Verify the implementation of
sols.Scan()to ensure it supports scanning into a map and correctly populates it with the solution bindings.docs/proto/logic.md (1)
- 379-379: The change from
Termtoexpressionin theSubstitutionentity is correctly documented. This aligns with the PR's objective to represent substitutions directly as Prolog term strings, enhancing flexibility and expressiveness.
Adopt expression for the values substitued represented as a prolog term. Previous format was an incorrect mix of structured/unstructured.
ccamel
force-pushed
the
refactor/substituion-expression
branch
from
5a3c040 to
6c4cb3d
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
x/logic/types/types.pb.gois excluded by:!**/*.pb.go
Files selected for processing (16)
- docs/proto/logic.md (2 hunks)
- proto/logic/v1beta2/types.proto (1 hunks)
- x/logic/keeper/grpc_query_ask_test.go (7 hunks)
- x/logic/keeper/interpreter.go (6 hunks)
- x/logic/predicate/address_test.go (4 hunks)
- x/logic/predicate/bank_test.go (26 hunks)
- x/logic/predicate/builtin_test.go (2 hunks)
- x/logic/predicate/crypto_test.go (12 hunks)
- x/logic/predicate/did_test.go (2 hunks)
- x/logic/predicate/encoding_test.go (2 hunks)
- x/logic/predicate/file_test.go (11 hunks)
- x/logic/predicate/json_test.go (17 hunks)
- x/logic/predicate/string_test.go (13 hunks)
- x/logic/predicate/uri_test.go (4 hunks)
- x/logic/testutil/logic.go (1 hunks)
- x/logic/wasm/types.go (3 hunks)
Files skipped from review as they are similar to previous changes (15)
- docs/proto/logic.md
- proto/logic/v1beta2/types.proto
- x/logic/keeper/grpc_query_ask_test.go
- x/logic/predicate/address_test.go
- x/logic/predicate/bank_test.go
- x/logic/predicate/builtin_test.go
- x/logic/predicate/crypto_test.go
- x/logic/predicate/did_test.go
- x/logic/predicate/encoding_test.go
- x/logic/predicate/file_test.go
- x/logic/predicate/json_test.go
- x/logic/predicate/string_test.go
- x/logic/predicate/uri_test.go
- x/logic/testutil/logic.go
- x/logic/wasm/types.go
Additional comments: 7
x/logic/keeper/interpreter.go (7)
- 4-6: The addition of the
stringsimport is justified by its use in the newqueryInterpreterfunction for reading the query string. The update to thecontextimport seems to be a reordering, which is fine as long as it follows the project's import organization conventions.- 9-9: The addition of the
github.com/ichiban/prolog/engineimport is necessary for the new Prolog engine functionalities introduced, such as parsing and executing Prolog queries. This aligns with the PR's objective to refactor the solution scanning process.- 53-67: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [43-75]
The refactoring of the
executemethod to usequeryInterpreterfor query execution simplifies the execution flow by abstracting the query interpretation logic into a separate function. This change potentially improves maintainability and readability. However, ensure that all error handling and context enhancements are preserved and correctly implemented in the new structure.
- 75-108: The introduction of
queryInterpreteris a significant change that centralizes the logic for executing queries and processing results. It's important to ensure that the logic for handling limits, parsing queries, and converting environments to results is correct and efficient. The use oflo.IfFfor conditional error message handling is a neat touch, but ensure that the librarygithub.com/samber/lois used consistently and effectively throughout the project.- 71-117: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [114-163]
The
newInterpreterfunction setup includes configuring predicates and virtual files filters based on parameters, which is a good practice for flexibility and configurability. However, ensure that the logic for handling default values and overrides is thoroughly tested, especially the use ofnonNilNorZeroOrDefaultUint64and the mapping of URLs. The creation of aBoundedBufferfor user output based on limits is a thoughtful addition for managing output size.
- 163-171: The
checkLimitsfunction adds a necessary validation step for query size against configured limits. This is a good practice for enforcing constraints and preventing potential abuse. Ensure that the error message provides clear and actionable information to the caller.- 197-222: Utility functions
parsedVarsToVarsandenvsToResultsare crucial for converting Prolog engine data structures to the application's domain types. It's important to ensure that the conversion logic correctly handles Prolog terms and environments, especially error handling during the scanning of expressions. The use ofstring(expression)in line 215 to convertprolog.TermStringto a Go string should be verified for correctness in representing Prolog terms.
|
🎉 This PR is included in version 7.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This PR implements the solution adopted from the discussion okp4/community#14, which is to use an unstructured format for substitutions of the
logicmodule by directly representing them as a Prolog term string (atom, number, or compound).I also seized the opportunity to refactor the scanning process of the solutions, thereby eliminating the use of go routines and channels in the ichiban/prolog lib (cf interpreter.go#L200).
Notes:
⚠️ This change introduces a breaking change in the
logicmodule contract.Summary by CodeRabbit
expressionto represent values in substitutions, offering more precise and flexible query results.UserOutputinAskResponseand anErrorfield inAnswerstruct for improved user feedback and error handling.testutil.TermResultstype, ensuring consistency and reliability in test results.expressioninstead oftermin various contexts, streamlining the handling of substitutions.