feat: add support for general object methods #29
Conversation
similarly to general object properties
WalkthroughThe changes reorganise and clarify the documentation for object dot accessors, add a new example demonstrating chained method calls, and improve the explanation of how Go objects are accessed within expressions. In the tests, a new function is added to verify chained method call parsing and evaluation, and an existing test is renamed. Code changes introduce an experimental Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GalExpressionParser
participant GalEvaluator
participant GoObject
User->>GalExpressionParser: Parse "aCar.GetThinger().Thing().Add('::with a suffix')"
GalExpressionParser->>GalEvaluator: Build AST for chained method calls
GalEvaluator->>GoObject: Call GetThinger()
GoObject-->>GalEvaluator: Returns Thinger object
GalEvaluator->>GoObject: Call Thing() on Thinger
GoObject-->>GalEvaluator: Returns Thing object
GalEvaluator->>GoObject: Call Add("::with a suffix") on Thing
GoObject-->>GalEvaluator: Returns final result string
GalEvaluator-->>User: Result: "it's a thing!::with a suffix"
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: can't load config: can't set severity rule option: no default severity defined ✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (2)
tree.go (1)
8-8: Consider replacing the entry type withanydirectly.The comment already suggests this change. Go's
anytype was introduced as a more readable alias forinterface{}and has become the preferred approach in modern Go code.-type entry interface{} // NOTE: this could be dropped in favour of using `any` directly. +type entry = anyobject.go (1)
12-17: New DotFunction struct provides foundation for enhanced method chaining.This experimental structure is well designed to encapsulate a method call with its receiver, name, function body, and arguments. While not yet integrated into the codebase (as noted in the comment), it provides a clear foundation for implementing the method chaining functionality that the tests now cover.
Consider documenting the intended usage pattern and providing an example to guide future development.
// Example of intended usage: // expr := `obj.Method1().Method2(arg)` // might be represented as: // DotFunction{ // Name: "Method2", // Receiver: DotFunction{ // Name: "Method1", // Receiver: objectVal, // BodyFn: method1Fn, // }, // Args: []Tree{{argValue}}, // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(2 hunks)gal_test.go(2 hunks)object.go(1 hunks)tree.go(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
object.go (3)
value.go (1)
Value(7-13)function.go (1)
FunctionalValue(12-12)tree.go (1)
Tree(10-10)
tree.go (4)
value_undefined.go (4)
Undefined(16-18)Undefined(91-93)Undefined(95-97)NewUndefinedWithReasonf(24-28)value.go (1)
Value(7-13)object.go (5)
ObjectValue(36-39)Object(29-29)ObjectGetMethod(183-285)Dot(21-23)ObjectGetProperty(124-180)variable.go (1)
Variable(3-5)
🔇 Additional comments (9)
tree.go (4)
99-100: Good optimization to fail fast on undefined values.Moving this check earlier prevents unnecessary processing when an undefined value is already detected. This is a sensible optimization.
207-221: Improved object receiver handling provides better type safety.The refactoring to use
anyfor the receiver and explicitly unwrapObjectValueobjects improves the accessor code in several ways:
- Better type representation (receiver is truly "any" type)
- More descriptive variable name ("receiver" vs "vVal")
- Explicit handling of ObjectValue unwrapping with clear comments
- Better error messages that guide the user to check if the receiver is nil
This change aligns with the PR's goal of extending support for general object methods and improves maintainability.
235-235: Simplified error return path improves readability.Directly returning the result of
vFv(which will be an Undefined type) streamlines the error handling path and removes the need for creating a new Undefined instance.
241-255: Consistent pattern applied to variable accessor function.The same improvement pattern from
objectAccessorDotFunctionFnhas been correctly applied toobjectAccessorDotVariableFn, ensuring consistency between both accessor functions.gal_test.go (3)
396-396: Consistent naming pattern applied to test function.The renamed test function now follows a consistent naming pattern with underscores between words, which improves readability and aligns with the new test added below.
541-592: Comprehensive test for chained method calls.This test effectively validates both parsing and evaluation of chained method calls, which is a key part of the PR's goal. The test:
- Verifies the correct AST structure for nested method calls
- Confirms evaluation produces the expected result
- Ensures the unwrapping of receivers works correctly through the chain
The test aligns perfectly with the updated README example and provides good coverage for the implemented functionality.
539-539:Details
❓ Verification inconclusive
Interesting exploration idea for future development.
The TODO comment suggests exploring nested parentheses expressions like
((aCar.Stereo).Brand).Country, which would be a valuable extension to test the robustness of the accessor implementation.To better understand the current limitations around parenthesized expressions, we could verify if the parser currently supports this syntax:
🏁 Script executed:
#!/bin/bash # Check for any existing tests with parenthesized object expressions grep -r "(\s*[a-zA-Z0-9_]*\.[a-zA-Z0-9_]*\s*)" --include="*.go" .Length of output: 4664
No nested parentheses property access tests found
I searched the existing Go tests for parenthesised field-access expressions (e.g.
((aCar.Stereo).Brand).Country) and didn’t find any examples. We should confirm whether our parser grammar already supports nested, parenthesised object accessors. If it doesn’t, extending the grammar and adding a dedicated test in gal_test.go (or a new tree_builder_test.go case) for((aCar.Stereo).Brand).Countrywill help verify and harden this functionality.• Verify parser support in the expression grammar (e.g. in tree.go or grammar definitions).
• Add a unit test covering a nested, parenthesised accessor expression.README.md (2)
225-228: Clearer explanation of dot accessor semantics.The revised description more concisely explains how gal handles object properties and methods through the dot accessor, and the automatic type conversion capabilities that support expression evaluation.
239-256: Well-structured examples clarify dot accessor usage patterns.The new examples section with distinct "Example 1" and "Example 2" headings provides clear, focused illustrations of both property access chains and method call chains. This organization makes the documentation more readable and the examples more discoverable.
Example 2 nicely demonstrates the method chaining functionality that was implemented in the code and tested in the new test function, showing good alignment between documentation and implementation.
similarly to general object properties
Summary by CodeRabbit
Documentation
Tests
Refactor