feat: remove entryType + add Calculate to Types #28
Conversation
WalkthroughThis change removes the custom Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Tree
participant Function/Variable/ObjectProperty/ObjectMethod
participant treeConfig
Caller->>Tree: Calc(val, op, cfg)
Tree->>+Function/Variable/ObjectProperty/ObjectMethod: Calculate(val, op, cfg)
Function/Variable/ObjectProperty/ObjectMethod->>treeConfig: (if needed) Lookup or evaluate value/body
Function/Variable/ObjectProperty/ObjectMethod-->>Tree: Result (possibly after operator application)
Tree-->>Caller: Final 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 ✨ 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: 7
🔭 Outside diff range comments (1)
tree.go (1)
221-236:⚠️ Potential issueIncorrect
%Tformatting in accessor error + duplicated logicBoth
objectAccessorDotFunctionFnandobjectAccessorDotVariableFnbuild the error message withfmt.Sprintf("%T", val)where%Talready expectsval.-return NewUndefinedWithReasonf( - "syntax error: object accessor function called on unknown or non-function member: [object: '%T'] [member: '%s']", - fmt.Sprintf("%T", val), fn.Name) +return NewUndefinedWithReasonf( + "syntax error: object accessor function called on unknown or non-function member: [object: '%T'] [member: '%s']", + val, fn.Name)Please apply the same change in
objectAccessorDotVariableFn.
🧹 Nitpick comments (4)
value_undefined.go (2)
91-97: Clarify error propagation & avoid potential self-reference confusionThe reformulated error message is clearer than the previous
kind()-based one – nice improvement.
One minor point: forothervalues that already represent anUndefined,other.String()will return"undefined"(or a longer reason). That is fine, but if the aim is to pinpoint the non-undefined operand, you might instead want to flag whenotheris notUndefined.Nothing blocking, just worth considering if the error text is meant to single out the valid operand.
110-115:IsUndefinedreturns the inverse of its name
IsUndefinedcurrently returnstruewhen no reason is set (u.reason == "").
This means anUndefinedwith a reason (the most common case in error paths) reports not undefined.Although this predates the refactor, it keeps tripping consumers that rely on the helper. Consider flipping the logic or renaming the method to avoid ambiguity.
tree.go (2)
113-118: Debug print left in production path
fmt.Printf("DEBUG - Tree.Calc: entry in Tree %T\n", typedE)will spam stdout during every evaluation.- fmt.Printf("DEBUG - Tree.Calc: entry in Tree %T\n", typedE) + // fmt.Printf removed – avoid noisy output in production
195-199: Unused//nolint:errcheckdirectives and false-positive linter error
calculatereturns onlyValue; thereforeerrcheckis irrelevant and the directives are now flagged as “unused”. Simply drop them:-//nolint:errcheck // life's too short to check for type assertion success hereIf you keep type assertions, prefer explicit
okchecks instead of silencing the linter.Also applies to: 292-293
🧰 Tools
🪛 GitHub Actions: Test and Build
[error] 195-195: Unused nolint directive for linter "errcheck" (nolintlint): //nolint:errcheck // life's too short to check for type assertion success here
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
function.go(1 hunks)object.go(4 hunks)operator.go(0 hunks)tree.go(9 hunks)value.go(1 hunks)value_bool.go(0 hunks)value_multivalue.go(0 hunks)value_number.go(0 hunks)value_number_test.go(0 hunks)value_string.go(0 hunks)value_undefined.go(1 hunks)variable.go(1 hunks)
💤 Files with no reviewable changes (6)
- value_number.go
- value_string.go
- value_number_test.go
- value_multivalue.go
- value_bool.go
- operator.go
🧰 Additional context used
🧬 Code Graph Analysis (4)
value_undefined.go (3)
value_bool.go (1)
Bool(5-8)value_string.go (1)
String(5-8)value.go (1)
Value(7-13)
object.go (5)
variable.go (1)
Variable(3-5)value.go (1)
Value(7-13)value_undefined.go (4)
NewUndefinedWithReasonf(24-28)Undefined(16-18)Undefined(91-93)Undefined(95-97)function.go (1)
NewFunction(25-31)tree_config.go (3)
WithFunctions(142-146)WithVariables(134-138)WithObjects(152-156)
function.go (4)
operator.go (1)
Operator(3-3)tree_config.go (3)
WithFunctions(142-146)WithVariables(134-138)WithObjects(152-156)value_undefined.go (3)
Undefined(16-18)Undefined(91-93)Undefined(95-97)value.go (1)
Value(7-13)
variable.go (3)
operator.go (1)
Operator(3-3)value_undefined.go (3)
Undefined(16-18)Undefined(91-93)Undefined(95-97)value.go (1)
Value(7-13)
🪛 GitHub Actions: Test and Build
function.go
[error] 49-49: Error return value is not checked (errcheck) at val = calculate(val.(Value), op, rhsVal)
variable.go
[error] 25-25: Error return value is not checked (errcheck) at val = calculate(val.(Value), op, rhsVal)
tree.go
[error] 190-190: Error return value is not checked (errcheck) at val = calculate(val.(Value), op, rhsVal)
[error] 195-195: Unused nolint directive for linter "errcheck" (nolintlint): //nolint:errcheck // life's too short to check for type assertion success here
[error] 292-292: Unused nolint directive for linter "errcheck" (nolintlint): //nolint:errcheck // life's too short to check for type assertion success here
🔇 Additional comments (1)
value.go (1)
3-5: Import change looks goodThe switch from a single-line to a multi-line import is syntactically correct and more future-proof should further imports be required.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
function.go (1)
33-53:⚠️ Potential issuePropagate
calculateerrors to callers (recurring issue)The error from
calculateon line 50 is silently discarded, failingerrcheckand masking run-time issues, as also noted in previous reviews.Apply this diff to properly handle type assertion and calculation errors:
- //nolint:errcheck // life's too short to check for type assertion success here - val = calculate(val.(Value), op, rhsVal) - - return val + lhs, ok := val.(Value) + if !ok { + return NewUndefinedWithReasonf( + "internal error: left-hand side to function '%s' is not a Value (got %T)", + f.Name, val, + ) + } + + result := calculate(lhs, op, rhsVal) + if u, ok := result.(Undefined); ok { + return NewUndefinedWithReasonf("function '%s': %s", f.Name, u.reason) + } + return resultThis improves error handling by:
- Checking type assertion success
- Propagating errors from calculation
- Adding context about which function caused the error
🧹 Nitpick comments (1)
tree.go (1)
149-150: Consider refactoring Dot[T] specializationsThe TODO comment suggests
Dot[Member]no longer makes sense as implementations of Calculate have diverged. This would be an opportunity to evaluate whether generic Dot[T] is still appropriate or if separate implementations would be clearer.Consider refactoring to separate concrete types like
DotFunctionandDotVariableif the implementations are significantly different.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
function.go(1 hunks)object.go(3 hunks)tree.go(6 hunks)variable.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- variable.go
- object.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
function.go (3)
tree_config.go (3)
WithFunctions(142-146)WithVariables(134-138)WithObjects(152-156)value_undefined.go (3)
Undefined(16-18)Undefined(91-93)Undefined(95-97)value.go (1)
Value(7-13)
🔇 Additional comments (2)
tree.go (2)
291-315: Fixed string formatting in Tree.String() methodThe previous tree formatting issue mentioned in past reviews has been resolved in the type switch implementation. The code now correctly handles each type with appropriate formatting verbs.
The refactoring to use Go type switches instead of enum-based switch makes the code more idiomatic and maintainable.
176-194: LGTM: New Tree.Calculate method implementationThe new
Calculatemethod follows a consistent pattern with other implementations, properly handling edge cases and preserving error context.
Summary by CodeRabbit