DotFunction #30
DotFunction #30
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis set of changes refactors the handling of object method and property access within the codebase. The previous approach, which used separate Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Tree
participant DotFunction
participant Object (Go object)
participant Config
User->>Tree: Evaluate expression with chained method calls
Tree->>DotFunction: Call Calculate(val, cfg)
DotFunction->>Object: Retrieve method by name via reflection
Object-->>DotFunction: Return method function (if found)
DotFunction->>DotFunction: Set BodyFn to retrieved method
DotFunction->>Config: Evaluate function with context
Config-->>DotFunction: Return result
DotFunction-->>Tree: Return result or Undefined
Tree-->>User: Return final evaluation result
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 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ 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: 3
♻️ Duplicate comments (1)
tree_builder_test.go (1)
188-211: (same as above) The repeated replacements are consistent and correct.
🧹 Nitpick comments (8)
tree.go (2)
144-146:DotFunction.Calculatediverges from the otherCalculatesignatures – verify nil-receiver handlingAll other entry types pass
(val, op, cfg)to theirCalculatemethod, butDotFunctionreceives only(val, cfg). The special-case is fine, yet:
- Call-sites (here and possibly elsewhere) must guarantee that
valis never nil when the accessor is evaluated, otherwise the method will have to detect and report “missing receiver” itself.- Long-term maintainability would benefit from a common interface (
Calculate(entry, Operator, *treeConfig)) for all calculable nodes.Please double-check that
DotFunction.Calculategracefully handlesval == niland document the expectation if it must not be nil.
271-273: Misleading log label: still prints “ObjectAccessor[Function]”The string output now refers to
typedE.String()but the labelObjectAccessor[Function]survived the previous generic naming (Dot[Function]). To keep debugging output accurate and avoid confusion with the new dedicated type, rename the label:- res += fmt.Sprintf("%sObjectAccessor[Function] %s\n", indent, typedE.String()) + res += fmt.Sprintf("%sDotFunction %s\n", indent, typedE.String())Minor, yet it pays off when tracing complex evaluation logs.
tree_config.go (1)
94-95: Out-of-date comment referenceThe comment mentions
Dot[Variable] / DotFunction; while correct, the previous part of the sentence still talks about using aVariable or a Functionfirst. Consider amending the whole sentence for clarity:“…then a
DotVariableorDotFunction”Pure documentation tweak, but helps future readers.
object_method.go (2)
16-22: Constructor arg name is misleading – usemethodNameinstead ofpropertyName.
The second parameter refers to a method but is namedpropertyName, which will confuse readers and IDE ref-tools.-func NewObjectMethod(objectName, propertyName string, args ...Tree) ObjectMethod { +func NewObjectMethod(objectName, methodName string, args ...Tree) ObjectMethod { return ObjectMethod{ - ObjectName: objectName, - MethodName: propertyName, + ObjectName: objectName, + MethodName: methodName, Args: args, } }
45-47: Minor: include arguments inStringfor easier debugging.Right now the string form is
aCar.CurrentSpeed; adding argument info helps when several overloads exist:return fmt.Sprintf("%s.%s(%d arg(s))", om.ObjectName, om.MethodName, len(om.Args))object.go (1)
22-27: Unchecked type assertion may return misleading “syntax error”.
val.(Value)is attempted after the earlierok := val.(Value); ifvalis not aValue, the function already returns anUndefinedbut the panic path is still reachable at line 40 whencalculateis called.Reuse the already-asserted
receivervariable to avoid the second assertion:- val = calculate(val.(Value), op, rhsVal) + val = calculate(receiver.(Value), op, rhsVal)tree_builder_test.go (1)
221-229: Readable but consider named field for clarity.Although legal, using an unnamed literal may puzzle newcomers. A tiny readability tweak:
gal.DotFunction{ Function: gal.NewFunction( ... ), },object_property.go (1)
23-37: Consider adding proper error handling for type assertions.The linter directive to ignore error checking for type assertions has a humorous comment, but this approach could lead to runtime panics. While this may be acceptable for now, proper error handling would improve robustness.
-//nolint:errcheck // life's too short to check for type assertion success here -func (o ObjectProperty) Calculate(val entry, op Operator, cfg *treeConfig) entry { +func (o ObjectProperty) Calculate(val entry, op Operator, cfg *treeConfig) entry { rhsVal := cfg.ObjectProperty(o) if u, ok := rhsVal.(Undefined); ok { return u } if val == nil { return rhsVal } - val = calculate(val.(Value), op, rhsVal) + lhsVal, ok := val.(Value) + if !ok { + return NewUndefinedWithReasonf("error: left-hand side value not a valid Value type when accessing %s", o) + } + val = calculate(lhsVal, op, rhsVal) return val }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
gal_test.go(1 hunks)object.go(3 hunks)object_method.go(1 hunks)object_property.go(1 hunks)tree.go(2 hunks)tree_builder.go(1 hunks)tree_builder_test.go(3 hunks)tree_config.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
tree_builder.go (2)
function.go (1)
Function(18-23)object.go (1)
DotFunction(12-12)
object_method.go (5)
tree.go (1)
Tree(10-10)operator.go (1)
Operator(3-3)function.go (2)
NewFunction(25-31)Eval(281-293)tree_config.go (3)
WithFunctions(142-146)WithVariables(134-138)WithObjects(152-156)value.go (1)
Value(7-13)
tree.go (1)
object.go (1)
DotFunction(12-12)
object.go (5)
function.go (2)
Function(18-23)Eval(281-293)value_undefined.go (4)
NewUndefinedWithReasonf(24-28)Undefined(16-18)Undefined(91-93)Undefined(95-97)value.go (1)
Value(7-13)tree_config.go (3)
WithFunctions(142-146)WithVariables(134-138)WithObjects(152-156)variable.go (1)
Variable(3-5)
🔇 Additional comments (5)
object.go (2)
14-19: Early “internal error” blocks legitimate reuse ofDotFunction.
DotFunction.Calculaterefuses to run whenBodyFnis pre-set, yet you set it later in the same method (line 39).
If a previously evaluatedDotFunctionis reused (e.g. in another branch) this guard will wrongly fail.Consider only erroring when
BodyFnbelongs to a built-in function (detectable viacfg.functions) or simply drop this check.
51-51: Generics cleanup looks good.Limiting
Memberto justVariablesimplifies the genericDotAPI without breaking current use-cases.tree_builder_test.go (1)
178-187: Test update matches the new API – good catch.Replacing the generic
Dot[Function]byDotFunctionkeeps the test aligned with the refactor and compiles cleanly with anonymous-field struct literals.gal_test.go (1)
551-568: Type change from generic to concrete looks good.The updated code correctly uses the new
DotFunctiontype which embedsFunctioninstead of the previous genericDot[Function]type. This aligns with the refactoring that consolidates method invocation logic into a single type.object_property.go (1)
7-14: Good structure for object property access.The
ObjectPropertytype is well-designed for representing object property access. The TODO comment raises an interesting point about potential unification with the pattern used forFunction.Receiver- consider exploring this in a future update.
Summary by CodeRabbit