Re-org files, update doc #24
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change refactors the handling of user-defined variables, functions, and objects by moving their definitions and related logic from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TreeConfig
participant Variables
participant Functions
participant Objects
User->>TreeConfig: Initialise with WithVariables, WithFunctions, WithObjects
User->>TreeConfig: Request Variable(name)
TreeConfig->>Variables: Get(name)
Variables-->>TreeConfig: Value or Undefined
User->>TreeConfig: Request Function(name)
TreeConfig->>Functions: Get(name)
Functions-->>TreeConfig: FunctionalValue or Undefined
User->>TreeConfig: Request ObjectProperty(objProp)
TreeConfig->>Objects: Get(objProp.ObjectName)
Objects-->>TreeConfig: Object or Undefined
User->>TreeConfig: Request ObjectMethod(objMethod)
TreeConfig->>Objects: Get(objMethod.ObjectName)
Objects-->>TreeConfig: Object or Undefined
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 (6)
✨ Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (2)
value.go (2)
46-57: 🛠️ Refactor suggestionError signal is swallowed – propagate the boolean from
toValue
ToValuediscards theokflag returned bytoValue, so callers can’t distinguish “successful conversion” from “fell back to Undefined”. This silently hides user mistakes.-func ToValue(value any) Value { - v, _ := toValue(value) - return v +func ToValue(value any) (Value, bool) { + return toValue(value) }Call-sites that genuinely want the old behaviour can ignore the second return value.
59-71:⚠️ Potential issueRisk of panic in helper conversions
ToNumberandToBoolrely on unchecked type assertions and will panic if a caller passes the wrong kind ofValue. Given these helpers are exported and very tempting to use, a single programmer error will bring the whole interpreter down.Consider returning
(Number, error)/(Bool, error)(or anUndefined) instead, keeping the engine robust.
♻️ Duplicate comments (1)
value_bool.go (1)
57-62: Same concern forOrSee note above regarding
And.
🧹 Nitpick comments (10)
tree_builder.go (1)
240-245: Minor phrasing nit.
gal.entryis not a public term elsewhere in the docs – consider simply “gal entry” (without the dot) or “gal.Tree entry” for clarity.
Purely editorial; no functional impact.README.md (2)
223-231: Spelling & clarity tweaks for the new Dot-accessor section.Small typos hinder readability:
-Objects Dot accessors +Objects – Dot accessors ... -While user-defined Objects are generally Value-centric, `gal` supports accessing porperties and methods on Go objects too +While user-defined Objects are generally Value-centric, `gal` also allows accessing properties and methods on plain Go objectsFixing “porperties” → “properties” and rephrasing the first sentence improves professional polish.
240-245: Hard tabs flagged by markdownlint (MD010).The code example uses real tab characters; replace them with spaces to satisfy common Markdown linters.
Low-priority, affects only documentation style.
value_bool.go (2)
14-24:NewBoolFromStringshould be case-insensitive and signal Undefined on bad inputAt present only the exact literals
"True"/"False"are accepted, which is surprising in a user-facing language. Treating"true","FALSE", etc. as synonyms and returning anUndefined(implementingValue) instead of anerrorwould align with the rest of the value system.-switch s { -case "True": +switch strings.ToLower(s) { +case "true": return True, nil -case "False": +case "false": return False, nil default: - return False, errors.Errorf("'%s' cannot be converted to a Bool", s) + return Undefined{}, errors.Errorf("'%s' is not a boolean literal", s) }
86-89: Exported sentinel values: avoid re-assignment
True/Falseare exported mutable vars. A package consumer could dogal.True = gal.Falseand break invariants. Prefer unexported vars plus exported functions (e.g.gal.True()returning aBool), or declare them asconstwith an underlying type.tree_config.go (3)
58-64: Unknown variable produces runtime Undefined – offer compile-time validation hook?
Variable()returnsUndefinedfor unknown names. While this is consistent with the rest of the engine, typos will only surface at evaluation time. Providing an optional “strict mode” or pre-evaluation validation pass that lists unresolved identifiers would greatly improve developer experience.
78-95:Function()dot-notation parsing: edge-cases & allocation
Multiple dots (
a.b.c) are currently treated as a hard error, but the message says “too many dot accessors: max 1 permitted”. If deeper chaining is ever required (nested structs), this will need a rethink; consider supporting arbitrary depth with reflection.
strings.Split(name, ".")allocates a slice every call.strings.Count+strings.Index/LastIndexcould avoid allocation in the common “no dot” path.
108-120:ObjectMethodduplicates logic already inFunction()Both methods perform object lookup, error wrapping and
ObjectGetMethodinvocation. Consolidating into a single helper would reduce maintenance burden and keep error semantics consistent.value_undefined.go (1)
95-103: Logical operators silently coerce tofalse; consider tri-state or propagate undefinedReturning
Falsefor bothAndandOrhides the error state and can lead to incorrect logical conclusions.
Options:
- Propagate
Undefined(return u)- Introduce a three-valued logic (
MaybeBool)- Document emphatically why
falseis the desired contract.At minimum, please document the choice in the code and unit-test the behaviour.
value_number.go (1)
223-240: Unbounded factorial loop can exhaust memory / CPUFor large inputs the loop:
for i.LessThanOrEqual(n.value) { … }will run for potentially millions of iterations and allocate gigantic decimals.
Consider:
- A sanity limit (e.g., n ≤ 1 000)
- Early bailout to
Undefinedwith a clear reason- Using a memoised or iterative approach with cut-offs
This guards both performance and denial-of-service vectors (user-supplied expressions).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
static/bit.ly_3MDZ9QT.pngis excluded by!**/*.png
📒 Files selected for processing (11)
README.md(3 hunks)gal.go(0 hunks)tree.go(1 hunks)tree_builder.go(1 hunks)tree_config.go(1 hunks)value.go(1 hunks)value_bool.go(1 hunks)value_multivalue.go(1 hunks)value_number.go(1 hunks)value_string.go(1 hunks)value_undefined.go(1 hunks)
💤 Files with no reviewable changes (1)
- gal.go
🧰 Additional context used
🧬 Code Graph Analysis (6)
value.go (2)
operator.go (12)
Multiply(17-17)Divide(18-18)LShift(21-21)RShift(22-22)LessThan(23-23)LessThanOrEqual(24-24)EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)And(29-29)Or(31-31)value_bool.go (2)
Bool(5-8)Bool(26-28)
tree.go (1)
value_undefined.go (1)
NewUndefinedWithReasonf(24-28)
tree_config.go (4)
value.go (1)
Value(5-28)function.go (1)
FunctionalValue(11-11)object.go (5)
Object(27-27)ObjectProperty(51-54)ObjectGetProperty(96-154)ObjectGetMethod(156-258)ObjectMethod(74-78)value_undefined.go (1)
NewUndefinedWithReasonf(24-28)
value_number.go (6)
value_undefined.go (13)
Undefined(16-18)Undefined(30-32)Undefined(63-65)Undefined(67-69)Undefined(71-73)Undefined(75-77)Undefined(79-81)Undefined(83-85)Undefined(87-89)Undefined(91-93)Undefined(95-98)Undefined(100-103)NewUndefinedWithReasonf(24-28)value.go (2)
Value(5-28)Numberer(34-36)value_string.go (3)
String(5-8)String(14-16)NewString(10-12)operator.go (10)
Multiply(17-17)Divide(18-18)LShift(21-21)RShift(22-22)LessThanOrEqual(24-24)LessThan(23-23)EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)function.go (9)
Floor(222-233)Sin(150-161)Cos(136-147)Sqrt(208-219)Tan(164-175)Ln(178-190)Log(193-205)Trunc(235-253)Factorial(124-133)value_bool.go (5)
Bool(5-8)Bool(26-28)NewBool(10-12)False(87-87)True(88-88)
value_multivalue.go (5)
value_undefined.go (13)
Undefined(16-18)Undefined(30-32)Undefined(63-65)Undefined(67-69)Undefined(71-73)Undefined(75-77)Undefined(79-81)Undefined(83-85)Undefined(87-89)Undefined(91-93)Undefined(95-98)Undefined(100-103)NewUndefinedWithReasonf(24-28)value.go (1)
Value(5-28)operator.go (1)
NotEqualTo(26-26)value_bool.go (1)
False(87-87)value_string.go (3)
String(5-8)String(14-16)NewString(10-12)
value_bool.go (5)
value_undefined.go (12)
Undefined(16-18)Undefined(30-32)Undefined(63-65)Undefined(67-69)Undefined(71-73)Undefined(75-77)Undefined(79-81)Undefined(83-85)Undefined(87-89)Undefined(91-93)Undefined(95-98)Undefined(100-103)operator.go (4)
EqualTo(25-25)NotEqualTo(26-26)And(29-29)Or(31-31)value.go (2)
Value(5-28)Booler(38-40)value_string.go (3)
String(5-8)String(14-16)NewString(10-12)value_number.go (3)
Number(10-13)Number(42-44)NewNumberFromInt(21-25)
🪛 markdownlint-cli2 (0.17.2)
README.md
234-234: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (3)
tree.go (1)
373-376: Good use of%Tfor clearer diagnostics.Switching from
%sto%Tsurfaces the concrete type behind the unknown dot-accessor, which should speed up debugging.
No further action needed.tree_builder.go (2)
240-240: Approve: Improved comment terminology
The comment now consistently refers to the “Dot” accessor, which aligns with the updated documentation and helps differentiate it from operator handling.
244-244: Approve: Clarified accessor usage context
The expanded remark clearly specifies that the dot accessor follows anygal.entryreturning a Go object, improving reader understanding of the parsing logic.
| func (b Bool) And(other Value) Bool { | ||
| if v, ok := other.(Booler); ok { | ||
| return NewBool(b.value && v.Bool().value) | ||
| } | ||
| return False // TODO: should Bool be a Maybe? | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Logical ops on non-bool operands silently degrade to False
When other is not a Booler, And returns False. This can mask programming errors: True And 42 suddenly becomes False with no hint something went wrong.
Returning an Undefined with a descriptive reason (or panicking during evaluation) would surface the misuse early.
- return False // TODO: should Bool be a Maybe?
+ return NewUndefinedWithReasonf("logical And requires Bool, got %T", other)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
value_multivalue.go (2)
34-38:⚠️ Potential issueEquality logic still inverted – identical elements return
falseThe previously-flagged issue has not been addressed.
NotEqualTo(..)==Falseis true when the elements are equal, so the
loop exits early and returnsfalseeven for identicalMultiValues.- if m.values[i].NotEqualTo(other.values[i]) == False { - return false + // if any element differs, the whole MultiValue is not equal + if m.values[i].NotEqualTo(other.values[i]) == True { + return false }
56-61:⚠️ Potential issueOff-by-one & negative-index guard missing in
GetThe check
i > len(m.values)permitsi == len(m.values)(out-of-range)
and silently ignores negative indices that will panic.- if i > len(m.values) { + if i < 0 || i >= len(m.values) { return NewUndefinedWithReasonf( "out of bounds: trying to get arg #%d on MultiValue that has %d arguments", i, len(m.values)) }value.go (1)
5-28: Interface is still too fat – consider capability-based decompositionRe-raising the earlier concern: every
Valueimplementer must fulfil ~20
methods, many of which are meaningless for certain types (e.g.Addon
Bool). This leads to no-op or panic stubs and brittle compile errors
whenever a new method is added.A more idiomatic Go design is to expose narrow interfaces (
Addable,
Comparable,Logical…) and have the evaluator demand them only when
it actually performs the corresponding operation.Ref: “accept interfaces, return structs” – keep contracts minimal.
value_string.go (3)
75-83: Guard against negative/non-integral repeat counts and potentialintoverflowThe current implementation doesn't validate the repeat count properly, which can lead to silent errors or panics. Additionally, the direct cast from
int64tointcould overflow on 32-bit architectures.- return String{ - value: strings.Repeat(s.value, int(v.Number().value.IntPart())), - } + count := v.Number().value + if !count.IsInteger() || count.IsNegative() { + return NewUndefinedWithReasonf("invalid repeat count: %s", count.String()) + } + n := count.IntPart() + if int64(int(n)) != n { // overflow check + return NewUndefinedWithReasonf("repeat count overflows on this architecture") + } + return String{value: strings.Repeat(s.value, int(n))}
85-101: Slice indices useint64, causing a compilation failure and potential out-of-range panicsThe current implementation doesn't properly handle bounds checking and uses an incorrect type for slice indices.
- return String{ - value: s.value[v.Number().value.IntPart():], - } + idx64 := v.Number().value.IntPart() + if idx64 < 0 || idx64 > int64(len(s.value)) { + return NewUndefinedWithReasonf("left shift out of range") + } + return String{value: s.value[int(idx64):]}
133-140:Number()panics on parse error – propagate asUndefinedinsteadExposing a panic from a method on a value type breaks the expression-evaluation contract: all failures elsewhere are surfaced as
Undefined.- n, err := NewNumberFromString(s.value) - if err != nil { - panic(err) // NOTE :-/ - } - return n + n, err := NewNumberFromString(s.value) + if err != nil { + return NewUndefinedWithReasonf("cannot convert %q to number: %v", s.value, err) + } + return n
🧹 Nitpick comments (3)
README.md (3)
187-191: Fix wording & punctuation in object-accessor introPlural and possessive forms are mixed up here and may confuse readers.
Consider re-phrasing for clarity:-Objects are Go `struct`'s which **properties** behave similarly to **gal variables** and **methods** to **gal functions**. +Objects are Go `struct`s whose **properties** behave like **gal variables** and whose **methods** act like **gal functions**.🧰 Tools
🪛 LanguageTool
[uncategorized] ~191-~191: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...ng the expression once withParseand runTree.Evalmultiple times with diffe...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
225-233: Correct typo “porperties” → “properties”Small spelling fix improves professionalism of the docs.
-While user-defined Objects are generally Value-centric, `gal` supports accessing porperties and methods on Go objects too, using the `.` accessor. +While user-defined Objects are generally Value-centric, `gal` supports accessing properties and methods on Go objects too, using the `.` accessor.
240-245: Replace hard tabs with spaces (markdownlint MD010)A static-analysis warning is raised for the hard tab in this fenced block.
Replacing it with spaces maintains visual alignment while satisfying linters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
static/bit.ly_3MDZ9QT.pngis excluded by!**/*.png
📒 Files selected for processing (11)
README.md(3 hunks)gal.go(0 hunks)tree.go(1 hunks)tree_builder.go(4 hunks)tree_config.go(1 hunks)value.go(1 hunks)value_bool.go(1 hunks)value_multivalue.go(1 hunks)value_number.go(1 hunks)value_string.go(1 hunks)value_undefined.go(1 hunks)
💤 Files with no reviewable changes (1)
- gal.go
✅ Files skipped from review due to trivial changes (1)
- tree_builder.go
🚧 Files skipped from review as they are similar to previous changes (5)
- tree.go
- value_bool.go
- tree_config.go
- value_number.go
- value_undefined.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
value_multivalue.go (4)
value_undefined.go (13)
Undefined(16-18)Undefined(30-32)Undefined(63-65)Undefined(67-69)Undefined(71-73)Undefined(75-77)Undefined(79-81)Undefined(83-85)Undefined(87-89)Undefined(91-93)Undefined(95-98)Undefined(100-103)NewUndefinedWithReasonf(24-28)value.go (1)
Value(5-28)value_bool.go (1)
False(87-87)value_string.go (3)
String(5-8)String(14-16)NewString(10-12)
value.go (2)
operator.go (12)
Multiply(17-17)Divide(18-18)LShift(21-21)RShift(22-22)LessThan(23-23)LessThanOrEqual(24-24)EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)And(29-29)Or(31-31)value_bool.go (2)
Bool(5-8)Bool(26-28)
🪛 markdownlint-cli2 (0.17.2)
README.md
234-234: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (2)
value.go (1)
59-71: Public helpers risk panics – return(val, ok)to callers
ToNumber,ToBooland similar helpers perform unchecked type assertions
that will panic at runtime for invalid conversions. Exposing a safe
version (returning(Number, bool)/(Bool, bool)) would let library
users handle errors gracefully and avoid surprising crashes.If keeping the current API, at least document the panic behaviour clearly.
value_string.go (1)
85-85: Add tests for LShift and RShift operationsThe TODOs indicate that tests are missing for these operations. Given the complex logic and potential edge cases, tests should be added to ensure correctness.
Could you please create tests for the LShift and RShift operations? Pay special attention to edge cases like:
- Empty strings
- Shifting by zero
- Shifting by the string length
- Shifting by more than the string length
- Shifting by negative values
- Shifting by non-integer values
Also applies to: 103-103
value_multivalue.go
Outdated
| // Equal satisfies the external Equaler interface such as in testify assertions and the cmp package | ||
| // Note that the current implementation defines equality as values matching and in order they appear. | ||
| func (m MultiValue) Equal(other MultiValue) bool { | ||
| if m.Size() != other.Size() { | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unit tests before widening API surface
The TODO notes hint at upcoming arithmetic/comparison support.
Before adding such methods, please introduce tests that cover:
- Construction with zero/one/many values
- Equality (happy-path & mismatch)
- Boundary conditions in
Get StringandAsStringformatting
Solid coverage will prevent regressions when the container gains more behaviour.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
value.go (1)
5-33: Interface still violates ISP – previous feedback remainsThe monolithic
Valueinterface continues to force every concrete type to implement the full arithmetic & logical surface, even when most operations are meaningless (e.g.PowerOfonBool). Narrow capability-based interfaces (Addable,Comparable, …) would avoid no-op or panic stubs and make future maintenance safer.value_bool.go (2)
50-55: Logical ops on non-bool operands silently returnFalseWhen
otheris not aBooler,AndreturnsFalse. This masks potential programming errors without warning - for example,True And 42would silently evaluate toFalserather than indicating an error.Consider returning an
Undefinedwith a descriptive reason:- return False // NOTE: should Bool be a Maybe? + return Bool{Undefined: NewUndefinedWithReasonf("logical And requires Bool operand, got %T", other)}
57-62: Logical ops on non-bool operands silently returnFalseThe same issue applies to the
Oroperation - it returnsFalsewhenotheris not aBooler, which can mask programming errors.Consider returning an
Undefinedwith a descriptive reason:- return False // NOTE: should Bool be a Maybe? + return Bool{Undefined: NewUndefinedWithReasonf("logical Or requires Bool operand, got %T", other)}value_string.go (3)
75-83: Guard against negative/non-integral repeat counts and potentialintoverflow
strings.Repeatwill panic if the count is negative or if the result exceeds memory limits. The current implementation:
- Silently truncates decimal values to integers
- Accepts negative inputs (causing panic)
- Performs unsafe
int64→intconversion which can overflow on 32-bit platforms- return String{ - value: strings.Repeat(s.value, int(v.Number().value.IntPart())), - } + count := v.Number().value + if !count.IsInteger() || count.IsNegative() { + return NewUndefinedWithReasonf("invalid repeat count: %s", count.String()) + } + n := count.IntPart() + if int64(int(n)) != n { // overflow check + return NewUndefinedWithReasonf("repeat count overflows on this architecture") + } + return String{value: strings.Repeat(s.value, int(n))}
86-101: Fix slice indices and bounds checking inLShiftThere are two critical issues with the string slicing:
stringslicing requiresintindices, butIntPart()returnsint64- No bounds checking is performed, so shifting beyond the string length will panic
- return String{ - value: s.value[v.Number().value.IntPart():], - } + idx64 := v.Number().value.IntPart() + if idx64 < 0 || idx64 > int64(len(s.value)) { + return NewUndefinedWithReasonf("left shift out of range") + } + return String{value: s.value[int(idx64):]}
104-119: Fix error messages and bounds checking inRShiftThe error messages incorrectly refer to "left shift" when this is
RShift. Additionally, the same bounds checking and type conversion issues exist here as inLShift.- if v.Number().value.IsNegative() { - return NewUndefinedWithReasonf("invalid negative left shift") - } - if !v.Number().value.IsInteger() { - return NewUndefinedWithReasonf("invalid non-integer left shift") - } + if v.Number().value.IsNegative() { + return NewUndefinedWithReasonf("invalid negative right shift") + } + if !v.Number().value.IsInteger() { + return NewUndefinedWithReasonf("invalid non-integer right shift") + } - return String{ - value: s.value[:int64(len(s.value))-v.Number().value.IntPart()], - } + shift := v.Number().value.IntPart() + limit := int64(len(s.value)) - shift + if shift < 0 || limit < 0 { + return NewUndefinedWithReasonf("right shift out of range") + } + return String{value: s.value[:int(limit)]}value_undefined.go (1)
63-93: Reason string is lost when propagating undefined resultsAll arithmetic and bit-shift methods return a brand-new
Undefined{}without preserving the originalreason. This loses important diagnostic information when chaining operations.Consider returning
u(or cloning with the same reason) to preserve the explanatory message:-func (u Undefined) Add(Value) Value { return Undefined{} } +func (u Undefined) Add(Value) Value { return u }Apply the same change to
Sub,Multiply,Divide,PowerOf,Mod,LShift, andRShift.value_number.go (2)
79-87:⚠️ Potential issueAdd division by zero check
decimal.Decimal.Divwill panic on a zero divisor. Before performing the division, you should check for zero and returnUndefined:- return Number{ - value: n.value.Div(v.Number().value), - } + if v.Number().value.IsZero() { + return NewUndefinedWithReasonf("division by zero") + } + return Number{ + value: n.value.Div(v.Number().value), + }
167-176:⚠️ Potential issueCheck for negative values in
Sqrt
big.Float.Sqrtof a negative number returnsNaN, whichdecimal.NewFromStringtreats as an error with the confusing message "invalid syntax". Add a negativity check to provide a clearer error message:+ if n.value.IsNegative() { + return NewUndefinedWithReasonf("square root of negative number") + } n, err := NewNumberFromString( new(big.Float).Sqrt(n.value.BigFloat()).String(), )
🧹 Nitpick comments (7)
tree.go (1)
450-458: Replace hard panic with controlled error propagationEven if the comment says this branch is unreachable,
panic()is a last resort and makes the evaluator hostile to embedding code. Returning anUndefinedwith a descriptive reason keeps the API pure‐function and lets callers decide how fatal the situation is.- panic("this point should never be reached") + return Tree{ + NewUndefinedWithReasonf("internal error: unhandled +/- consolidation state"), + }README.md (1)
223-246: Minor copy-editing & Markdown-lint issues
- “porperties” → “properties”.
- The new code block (line 234) contains a hard tab, flagged by markdown-lint – replace with spaces to keep formatting consistent.
-While user-defined Objects are generally Value-centric, `gal` supports accessing porperties and methods on Go objects too, using the `.` accessor. +While user-defined Objects are generally Value-centric, `gal` supports accessing properties and methods on Go objects too, using the `.` accessor.No functional impact, but polishing docs helps adoption.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
234-234: Hard tabs
Column: 1(MD010, no-hard-tabs)
value.go (2)
3-4: Orphaned import
fmtis only used by the embeddedfmt.Stringerconstraint in theValueinterface—so the import is necessary, but a short comment would prevent future “unused import” churn if that line gets refactored.import "fmt" // needed for fmt.Stringer in Value
52-53: Ignoring the ok flag loses actionable diagnostics
toValuealready tells you when conversion failed; dropping its boolean result guarantees that upstream call-sites can’t differentiate success from “Undefined with reason”. At minimum, document this deliberate choice, or bubble the flag up.value_bool.go (2)
35-40: Consider returningUndefinedfor equality comparisons with incompatible typesSimilar to the logical operations,
EqualTosilently returnsFalsewhen comparing with a non-boolean value, which could mask type errors. For consistency with the behaviour of other operations, consider returning anUndefinedvalue.- return False + return Bool{Undefined: NewUndefinedWithReasonf("cannot compare Bool with %T", other)}
14-24: Consider implementing the NOTE suggestion forNewBoolFromStringThe NOTE comment suggests returning a
Valueto allow for returningUndefinedwhen neither "True" nor "False" is provided. This would make the API more consistent with the rest of the codebase where errors are represented asUndefinedvalues.-// NOTE: another option would be to return a Value and hence allow Undefined when neither True nor False is provided. -func NewBoolFromString(s string) (Bool, error) { +func NewBoolFromString(s string) Value { switch s { case "True": return True, nil case "False": return False, nil default: - return False, errors.Errorf("'%s' cannot be converted to a Bool", s) + return Bool{Undefined: NewUndefinedWithReasonf("'%s' cannot be converted to a Bool", s)} } }value_string.go (1)
85-101: Add tests forLShiftandRShiftas noted in TODO commentsBoth
LShiftandRShiftoperations have TODO comments indicating tests should be added. These operations have known issues with bounds checking and type conversion, so tests are particularly important to verify the corrected behavior.func TestStringLShift(t *testing.T) { tests := []struct { name string str string shift Value expected Value }{ {"valid shift", "hello", NewNumberFromInt(2), NewString("llo")}, {"zero shift", "hello", NewNumberFromInt(0), NewString("hello")}, {"max shift", "hello", NewNumberFromInt(5), NewString("")}, {"negative shift", "hello", NewNumberFromInt(-1), NewUndefined()}, // should return Undefined {"non-integer shift", "hello", NewNumberFromFloat(2.5), NewUndefined()}, // should return Undefined {"out of bounds", "hello", NewNumberFromInt(10), NewUndefined()}, // should return Undefined } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { s := NewString(tc.str) result := s.LShift(tc.shift) // Compare result with expected value }) } }Would you like me to create these test cases for you?
Also applies to: 103-119
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
static/bit.ly_3MDZ9QT.pngis excluded by!**/*.png
📒 Files selected for processing (11)
README.md(3 hunks)gal.go(0 hunks)tree.go(2 hunks)tree_builder.go(4 hunks)tree_config.go(1 hunks)value.go(3 hunks)value_bool.go(1 hunks)value_multivalue.go(1 hunks)value_number.go(1 hunks)value_string.go(1 hunks)value_undefined.go(1 hunks)
💤 Files with no reviewable changes (1)
- gal.go
✅ Files skipped from review due to trivial changes (1)
- tree_builder.go
🚧 Files skipped from review as they are similar to previous changes (2)
- value_multivalue.go
- tree_config.go
🧰 Additional context used
🧬 Code Graph Analysis (4)
tree.go (1)
value_undefined.go (1)
NewUndefinedWithReasonf(24-28)
value.go (5)
operator.go (12)
Multiply(17-17)Divide(18-18)LShift(21-21)RShift(22-22)LessThan(23-23)LessThanOrEqual(24-24)EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)And(29-29)Or(31-31)value_bool.go (2)
Bool(5-8)Bool(26-28)value_number.go (2)
Number(10-13)Number(42-44)value_undefined.go (13)
Undefined(16-18)Undefined(30-32)Undefined(63-65)Undefined(67-69)Undefined(71-73)Undefined(75-77)Undefined(79-81)Undefined(83-85)Undefined(87-89)Undefined(91-93)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)value_string.go (2)
String(5-8)String(14-16)
value_bool.go (5)
value_undefined.go (12)
Undefined(16-18)Undefined(30-32)Undefined(63-65)Undefined(67-69)Undefined(71-73)Undefined(75-77)Undefined(79-81)Undefined(83-85)Undefined(87-89)Undefined(91-93)Undefined(95-97)Undefined(99-101)operator.go (4)
EqualTo(25-25)NotEqualTo(26-26)And(29-29)Or(31-31)value.go (2)
Value(5-33)Booler(43-45)value_string.go (3)
String(5-8)String(14-16)NewString(10-12)value_number.go (3)
Number(10-13)Number(42-44)NewNumberFromInt(21-25)
value_number.go (6)
value_undefined.go (13)
Undefined(16-18)Undefined(30-32)Undefined(63-65)Undefined(67-69)Undefined(71-73)Undefined(75-77)Undefined(79-81)Undefined(83-85)Undefined(87-89)Undefined(91-93)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)value.go (2)
Value(5-33)Numberer(39-41)value_string.go (3)
String(5-8)String(14-16)NewString(10-12)operator.go (10)
Multiply(17-17)Divide(18-18)LShift(21-21)RShift(22-22)LessThanOrEqual(24-24)LessThan(23-23)EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)function.go (9)
Floor(222-233)Sin(150-161)Cos(136-147)Sqrt(208-219)Tan(164-175)Ln(178-190)Log(193-205)Trunc(235-253)Factorial(124-133)value_bool.go (5)
Bool(5-8)Bool(26-28)NewBool(10-12)False(87-87)True(88-88)
🪛 markdownlint-cli2 (0.17.2)
README.md
234-234: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (3)
tree.go (1)
373-376: Improved diagnostic – 👍Switching from
%sto%Tgives the caller the concrete Go type, which is far more useful when debugging an "unknown dot-accessor" situation. Nothing else to flag here.README.md (2)
90-96: Wording fix looks goodSwitching to “use of general types” reads better and removes ambiguity. ✅
185-189: Clarification is valuableStating explicitly that struct properties behave like variables (rather than are variables) makes the mental model clearer for users—nice improvement.
| func ToNumber(val Value) Number { | ||
| //nolint:errcheck // life's too short to check for type assertion success here | ||
| return val.(Numberer).Number() // may panic | ||
| n, ok := val.(Numberer) | ||
| if !ok { | ||
| return Number{Undefined: NewUndefinedWithReasonf("value type %T - cannot convert to Number", val)} | ||
| } | ||
| return n.Number() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Returning a pseudo-Number hides error states
ToNumber wraps the failure inside a Number that still advertises kind() == valueEntryKind.
Down-stream code may assume it holds a valid decimal and attempt arithmetic, silently propagating the error.
Consider returning Undefined directly (changing the signature to Value) or introducing a MaybeNumber wrapper so callers must disambiguate success/failure.
-func ToNumber(val Value) Number {
+// Returning Value forces the caller to confront undefined cases.
+func ToNumber(val Value) Value {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func ToNumber(val Value) Number { | |
| //nolint:errcheck // life's too short to check for type assertion success here | |
| return val.(Numberer).Number() // may panic | |
| n, ok := val.(Numberer) | |
| if !ok { | |
| return Number{Undefined: NewUndefinedWithReasonf("value type %T - cannot convert to Number", val)} | |
| } | |
| return n.Number() | |
| } | |
| // Returning Value forces the caller to confront undefined cases. | |
| func ToNumber(val Value) Value { | |
| n, ok := val.(Numberer) | |
| if !ok { | |
| return Number{Undefined: NewUndefinedWithReasonf("value type %T - cannot convert to Number", val)} | |
| } | |
| return n.Number() | |
| } |
| func (u Undefined) IsUndefined() bool { | ||
| return u.reason == "" // NOTE: this is not quite accurate: an Undefined may not hold a reason | ||
| } |
There was a problem hiding this comment.
Inaccurate implementation of IsUndefined()
As noted in the comment, this implementation is not accurate. An Undefined value is always undefined regardless of whether it has a reason. This method currently returns false for an Undefined with a reason, which is misleading.
-func (u Undefined) IsUndefined() bool {
- return u.reason == "" // NOTE: this is not quite accurate: an Undefined may not hold a reason
-}
+func (u Undefined) IsUndefined() bool {
+ return true // An Undefined is always undefined
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (u Undefined) IsUndefined() bool { | |
| return u.reason == "" // NOTE: this is not quite accurate: an Undefined may not hold a reason | |
| } | |
| func (u Undefined) IsUndefined() bool { | |
| return true // An Undefined is always undefined | |
| } |
| func (n Number) Log(precision int32) Value { | ||
| res, err := n.value.Ln(precision + 1) | ||
| if err != nil { | ||
| return NewUndefinedWithReasonf("Log:%s", err.Error()) | ||
| } | ||
|
|
||
| res10, err := decimal.New(10, 0).Ln(precision + 1) | ||
| if err != nil { | ||
| return NewUndefinedWithReasonf("Log:%s", err.Error()) | ||
| } | ||
|
|
||
| return Number{ | ||
| value: res.Div(res10).Truncate(precision), | ||
| } | ||
| } |
There was a problem hiding this comment.
Add base 10 check in Log implementation
The Log implementation calculates the log base 10 by dividing the natural log by the natural log of 10. However, if the input is 0 or negative, then this will lead to an error. Add a check for this case:
+ if n.value.IsZero() || n.value.IsNegative() {
+ return NewUndefinedWithReasonf("logarithm of non-positive number")
+ }
res, err := n.value.Ln(precision + 1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (n Number) Log(precision int32) Value { | |
| res, err := n.value.Ln(precision + 1) | |
| if err != nil { | |
| return NewUndefinedWithReasonf("Log:%s", err.Error()) | |
| } | |
| res10, err := decimal.New(10, 0).Ln(precision + 1) | |
| if err != nil { | |
| return NewUndefinedWithReasonf("Log:%s", err.Error()) | |
| } | |
| return Number{ | |
| value: res.Div(res10).Truncate(precision), | |
| } | |
| } | |
| func (n Number) Log(precision int32) Value { | |
| if n.value.IsZero() || n.value.IsNegative() { | |
| return NewUndefinedWithReasonf("logarithm of non-positive number") | |
| } | |
| res, err := n.value.Ln(precision + 1) | |
| if err != nil { | |
| return NewUndefinedWithReasonf("Log:%s", err.Error()) | |
| } | |
| res10, err := decimal.New(10, 0).Ln(precision + 1) | |
| if err != nil { | |
| return NewUndefinedWithReasonf("Log:%s", err.Error()) | |
| } | |
| return Number{ | |
| value: res.Div(res10).Truncate(precision), | |
| } | |
| } |
| func (n Number) Mod(other Value) Value { | ||
| if v, ok := other.(Numberer); ok { | ||
| return Number{ | ||
| value: n.value.Mod(v.Number().value), | ||
| } | ||
| } | ||
|
|
||
| return NewUndefinedWithReasonf("NaN: %s", other.String()) | ||
| } |
There was a problem hiding this comment.
Add modulo by zero check
Similar to division, Mod operation will fail if the divisor is zero. Add a check to handle this case gracefully:
+ if v.Number().value.IsZero() {
+ return NewUndefinedWithReasonf("modulo by zero")
+ }
return Number{
value: n.value.Mod(v.Number().value),
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (n Number) Mod(other Value) Value { | |
| if v, ok := other.(Numberer); ok { | |
| return Number{ | |
| value: n.value.Mod(v.Number().value), | |
| } | |
| } | |
| return NewUndefinedWithReasonf("NaN: %s", other.String()) | |
| } | |
| func (n Number) Mod(other Value) Value { | |
| if v, ok := other.(Numberer); ok { | |
| if v.Number().value.IsZero() { | |
| return NewUndefinedWithReasonf("modulo by zero") | |
| } | |
| return Number{ | |
| value: n.value.Mod(v.Number().value), | |
| } | |
| } | |
| return NewUndefinedWithReasonf("NaN: %s", other.String()) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (10)
value_multivalue.go (3)
13-13: Consider adding test cases before implementing arithmetic methods.The TODO comment suggests adding methods like
AddandLessThan. Before implementing these, consider adding comprehensive tests to define expected behaviour for edge cases.
35-38:⚠️ Potential issueFix inverted equality logic in
Equalmethod.The current implementation has inverted logic - it will return
falsewhen elements are equal because it's checkingNotEqualToequalsFalse. This will cause all equal MultiValue comparisons to fail.- if m.values[i].NotEqualTo(other.values[i]) == False { - return false + if m.values[i].NotEqualTo(other.values[i]) == True { + return false // Elements don't match, so MultiValues aren't equal
56-61:⚠️ Potential issueFix out-of-bounds check in
Getmethod.Two issues:
- The current bounds check (
i > len(m.values)) still allows an out-of-range access wheni == len(m.values).- There's no protection against negative indices, which will cause panics.
- if i > len(m.values) { + if i < 0 || i >= len(m.values) { return NewUndefinedWithReasonf("out of bounds: trying to get arg #%d on MultiValue that has %d arguments", i, len(m.values)) }value_string.go (3)
75-83:⚠️ Potential issueAdd safeguards against negative/non-integral repeat counts and potential integer overflow.
The
Multiplymethod doesn't validate that the number is positive and doesn't check for potential overflow when converting fromint64toint.-func (s String) Multiply(other Value) Value { - if v, ok := other.(Numberer); ok { - return String{ - value: strings.Repeat(s.value, int(v.Number().value.IntPart())), - } - } - - return NewUndefinedWithReasonf("NaN: %s", other.String()) +func (s String) Multiply(other Value) Value { + if v, ok := other.(Numberer); ok { + count := v.Number().value + if !count.IsInteger() || count.IsNegative() { + return NewUndefinedWithReasonf("invalid repeat count: %s", count.String()) + } + n := count.IntPart() + if int64(int(n)) != n { // overflow check + return NewUndefinedWithReasonf("repeat count overflows on this architecture") + } + return String{value: strings.Repeat(s.value, int(n))} + } + + return NewUndefinedWithReasonf("NaN: %s", other.String()) }
85-101:⚠️ Potential issueFix slice indices and bounds checking in
LShiftmethod.The
LShiftmethod has two issues:
- It uses
int64as a string index without conversion toint- There's no bounds checking, so an index beyond the string length will panic
-func (s String) LShift(other Value) Value { - if v, ok := other.(Numberer); ok { - if v.Number().value.IsNegative() { - return NewUndefinedWithReasonf("invalid negative left shift") - } - if !v.Number().value.IsInteger() { - return NewUndefinedWithReasonf("invalid non-integer left shift") - } - - return String{ - value: s.value[v.Number().value.IntPart():], - } - } - - return NewUndefinedWithReasonf("NaN: %s", other.String()) +func (s String) LShift(other Value) Value { + if v, ok := other.(Numberer); ok { + if v.Number().value.IsNegative() { + return NewUndefinedWithReasonf("invalid negative left shift") + } + if !v.Number().value.IsInteger() { + return NewUndefinedWithReasonf("invalid non-integer left shift") + } + + idx := v.Number().value.IntPart() + if idx < 0 || idx > int64(len(s.value)) { + return NewUndefinedWithReasonf("left shift out of range") + } + + return String{value: s.value[int(idx):]} + } + + return NewUndefinedWithReasonf("NaN: %s", other.String()) }
103-119:⚠️ Potential issueFix incorrect error messages and bounds checking in
RShiftmethod.The
RShiftmethod has several issues:
- Error messages incorrectly reference "left shift" instead of "right shift"
- No bounds checking for the resulting slice index
- Potential integer overflow with large strings
-func (s String) RShift(other Value) Value { - if v, ok := other.(Numberer); ok { - if v.Number().value.IsNegative() { - return NewUndefinedWithReasonf("invalid negative left shift") - } - if !v.Number().value.IsInteger() { - return NewUndefinedWithReasonf("invalid non-integer left shift") - } - - return String{ - value: s.value[:int64(len(s.value))-v.Number().value.IntPart()], - } - } - - return NewUndefinedWithReasonf("NaN: %s", other.String()) +func (s String) RShift(other Value) Value { + if v, ok := other.(Numberer); ok { + if v.Number().value.IsNegative() { + return NewUndefinedWithReasonf("invalid negative right shift") + } + if !v.Number().value.IsInteger() { + return NewUndefinedWithReasonf("invalid non-integer right shift") + } + + shift := v.Number().value.IntPart() + limit := int64(len(s.value)) - shift + if shift < 0 || limit < 0 { + return NewUndefinedWithReasonf("right shift out of range") + } + + return String{value: s.value[:int(limit)]} + } + + return NewUndefinedWithReasonf("NaN: %s", other.String()) }value_number.go (4)
79-87:⚠️ Potential issueAdd division by zero guard to prevent panic
The
Dividemethod should check for a zero divisor before performing the division operation. As it stands,decimal.Decimal.Divwill panic when dividing by zero. This could lead to application crashes.func (n Number) Divide(other Value) Value { if v, ok := other.(Numberer); ok { + if v.Number().value.IsZero() { + return NewUndefinedWithReasonf("division by zero") + } return Number{ value: n.value.Div(v.Number().value), } } return NewUndefinedWithReasonf("NaN: %s", other.String()) }
99-107:⚠️ Potential issueAdd modulo by zero check to prevent failure
The
Modoperation fails if the divisor is zero, similar to division. A check should be added to handle this case gracefully and return an appropriate error message.func (n Number) Mod(other Value) Value { if v, ok := other.(Numberer); ok { + if v.Number().value.IsZero() { + return NewUndefinedWithReasonf("modulo by zero") + } return Number{ value: n.value.Mod(v.Number().value), } } return NewUndefinedWithReasonf("NaN: %s", other.String()) }
167-176:⚠️ Potential issueAdd check for square root of negative numbers
The
Sqrtmethod should check for negative inputs before calculating the square root. Currently,big.Float.Sqrtof a negative number returnsNaN, whichdecimal.NewFromStringtreats as an error, leading to a confusing error message.func (n Number) Sqrt() Value { + if n.value.IsNegative() { + return NewUndefinedWithReasonf("square root of negative number") + } n, err := NewNumberFromString( new(big.Float).Sqrt(n.value.BigFloat()).String(), ) if err != nil { return NewUndefinedWithReasonf("Sqrt:%s", err.Error()) } return n }
195-209:⚠️ Potential issueAdd check for logarithm of non-positive numbers in Log implementation
The
Logimplementation calculates the log base 10 by dividing the natural log by the natural log of 10. However, if the input is 0 or negative, then this will lead to an error. Add a check for this case.func (n Number) Log(precision int32) Value { + if n.value.IsZero() || n.value.IsNegative() { + return NewUndefinedWithReasonf("logarithm of non-positive number") + } res, err := n.value.Ln(precision + 1) if err != nil { return NewUndefinedWithReasonf("Log:%s", err.Error()) } res10, err := decimal.New(10, 0).Ln(precision + 1) if err != nil { return NewUndefinedWithReasonf("Log:%s", err.Error()) } return Number{ value: res.Div(res10).Truncate(precision), } }
🧹 Nitpick comments (6)
README.md (1)
225-225: Fix typo in property naming.There's a typo in "porperties" which should be "properties".
-While user-defined Objects are generally Value-centric, `gal` supports accessing porperties and methods on Go objects too, using the `.` accessor. +While user-defined Objects are generally Value-centric, `gal` supports accessing properties and methods on Go objects too, using the `.` accessor.value_string.go (1)
142-149: Evaluate strings effectively, but consider error details.The
Evalmethod attempts to parse and evaluate the string, falling back to returning the original string if parsing fails. Consider returning the error information for debugging purposes.-func (s String) Eval() Value { - tree, err := NewTreeBuilder().FromExpr(s.value) - if err != nil { - return s - } - - return tree.Eval() +func (s String) Eval() Value { + tree, err := NewTreeBuilder().FromExpr(s.value) + if err != nil { + // Consider returning Undefined with error details for diagnostics + // return NewUndefinedWithReasonf("evaluation failed: %s", err.Error()) + return s + } + + return tree.Eval() }value_number.go (4)
56-57: More descriptive error message neededThe error message "NaN: %s" is not clearly indicating that the operation expects a numeric value. A more descriptive message would improve debugging.
- return NewUndefinedWithReasonf("NaN: %s", other.String()) + return NewUndefinedWithReasonf("Add operation requires a number, got: %s", other.String())
66-67: More descriptive error message neededSimilar to the Add method, the error message for Sub should be more descriptive.
- return NewUndefinedWithReasonf("NaN: %s", other.String()) + return NewUndefinedWithReasonf("Subtraction operation requires a number, got: %s", other.String())
76-77: More descriptive error message neededSimilar to other arithmetic operations, the error message for Multiply should be more descriptive.
- return NewUndefinedWithReasonf("NaN: %s", other.String()) + return NewUndefinedWithReasonf("Multiplication operation requires a number, got: %s", other.String())
42-44: Consider adding documentation for the kind methodThe
kindmethod is implementing an interface but lacks documentation. Adding a comment would improve code clarity.+// kind returns the type of entry this value represents func (Number) kind() entryKind { return valueEntryKind }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
static/bit.ly_3MDZ9QT.pngis excluded by!**/*.png
📒 Files selected for processing (12)
README.md(3 hunks)gal.go(0 hunks)object.go(2 hunks)tree.go(2 hunks)tree_builder.go(4 hunks)tree_config.go(1 hunks)value.go(3 hunks)value_bool.go(1 hunks)value_multivalue.go(1 hunks)value_number.go(1 hunks)value_string.go(1 hunks)value_undefined.go(1 hunks)
💤 Files with no reviewable changes (1)
- gal.go
✅ Files skipped from review due to trivial changes (2)
- object.go
- tree_builder.go
🚧 Files skipped from review as they are similar to previous changes (4)
- tree.go
- tree_config.go
- value_bool.go
- value_undefined.go
🧰 Additional context used
🧬 Code Graph Analysis (4)
value.go (5)
operator.go (12)
Multiply(17-17)Divide(18-18)LShift(21-21)RShift(22-22)LessThan(23-23)LessThanOrEqual(24-24)EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)And(29-29)Or(31-31)value_bool.go (2)
Bool(5-8)Bool(27-29)value_number.go (2)
Number(10-13)Number(42-44)value_undefined.go (13)
Undefined(16-18)Undefined(30-32)Undefined(63-65)Undefined(67-69)Undefined(71-73)Undefined(75-77)Undefined(79-81)Undefined(83-85)Undefined(87-89)Undefined(91-93)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)value_string.go (2)
String(5-8)String(14-16)
value_string.go (6)
value_undefined.go (13)
Undefined(16-18)Undefined(30-32)Undefined(63-65)Undefined(67-69)Undefined(71-73)Undefined(75-77)Undefined(79-81)Undefined(83-85)Undefined(87-89)Undefined(91-93)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)value.go (3)
Value(5-33)Stringer(35-37)Numberer(39-41)value_bool.go (4)
Bool(5-8)Bool(27-29)NewBool(10-12)False(88-88)value_number.go (3)
Number(10-13)Number(42-44)NewNumberFromString(33-40)function.go (1)
Eval(255-267)tree_builder.go (1)
NewTreeBuilder(11-13)
value_multivalue.go (4)
value_undefined.go (13)
Undefined(16-18)Undefined(30-32)Undefined(63-65)Undefined(67-69)Undefined(71-73)Undefined(75-77)Undefined(79-81)Undefined(83-85)Undefined(87-89)Undefined(91-93)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)value.go (1)
Value(5-33)value_bool.go (1)
False(88-88)value_string.go (3)
String(5-8)String(14-16)NewString(10-12)
value_number.go (6)
value_undefined.go (13)
Undefined(16-18)Undefined(30-32)Undefined(63-65)Undefined(67-69)Undefined(71-73)Undefined(75-77)Undefined(79-81)Undefined(83-85)Undefined(87-89)Undefined(91-93)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)value.go (2)
Value(5-33)Numberer(39-41)value_string.go (3)
String(5-8)String(14-16)NewString(10-12)operator.go (10)
Multiply(17-17)Divide(18-18)LShift(21-21)RShift(22-22)LessThanOrEqual(24-24)LessThan(23-23)EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)function.go (9)
Floor(222-233)Sin(150-161)Cos(136-147)Sqrt(208-219)Tan(164-175)Ln(178-190)Log(193-205)Trunc(235-253)Factorial(124-133)value_bool.go (5)
Bool(5-8)Bool(27-29)NewBool(10-12)False(88-88)True(89-89)
🪛 markdownlint-cli2 (0.17.2)
README.md
234-234: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (5)
README.md (1)
237-245: Clear explanation of dot accessor limitations.The explanation of dot accessor limitations and the difference between accessors vs operators is well-articulated. This helps users understand why certain syntactic constructs won't work.
value.go (2)
64-70: Improved error handling in ToNumber function.The updated implementation safely checks for the
Numbererinterface and handles type conversion errors correctly, returning a properly formatted Undefined value rather than panicking.
76-82: Consistent error handling in ToBool function.The updated implementation matches the approach in ToNumber, safely checking for the Booler interface and providing a clear error message when conversion fails.
value_number.go (2)
46-49: LGTM - Good practice for testing equalityThe
Equalmethod satisfies external equality interfaces which is good practice for testing and comparison operations.
223-240: LGTM - Proper validation in Factorial methodThe Factorial method correctly validates that the input is a non-negative integer before computation, which is the proper mathematical domain for factorials. The implementation is also efficient using a straightforward iterative approach.
| func (n Number) PowerOf(other Value) Value { | ||
| if v, ok := other.(Numberer); ok { | ||
| return Number{ | ||
| value: n.value.Pow(v.Number().value), | ||
| } | ||
| } | ||
|
|
||
| return NewUndefinedWithReasonf("NaN: %s", other.String()) | ||
| } |
There was a problem hiding this comment.
Add check for raising zero to negative power
The PowerOf method needs to handle the mathematical undefined case of raising zero to a negative power, which would result in division by zero.
func (n Number) PowerOf(other Value) Value {
if v, ok := other.(Numberer); ok {
+ if n.value.IsZero() && v.Number().value.IsNegative() {
+ return NewUndefinedWithReasonf("cannot raise zero to a negative power")
+ }
return Number{
value: n.value.Pow(v.Number().value),
}
}
return NewUndefinedWithReasonf("NaN: %s", other.String())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (n Number) PowerOf(other Value) Value { | |
| if v, ok := other.(Numberer); ok { | |
| return Number{ | |
| value: n.value.Pow(v.Number().value), | |
| } | |
| } | |
| return NewUndefinedWithReasonf("NaN: %s", other.String()) | |
| } | |
| func (n Number) PowerOf(other Value) Value { | |
| if v, ok := other.(Numberer); ok { | |
| if n.value.IsZero() && v.Number().value.IsNegative() { | |
| return NewUndefinedWithReasonf("cannot raise zero to a negative power") | |
| } | |
| return Number{ | |
| value: n.value.Pow(v.Number().value), | |
| } | |
| } | |
| return NewUndefinedWithReasonf("NaN: %s", other.String()) | |
| } |
| func (n Number) Ln(precision int32) Value { | ||
| res, err := n.value.Ln(precision) | ||
| if err != nil { | ||
| return NewUndefinedWithReasonf("Ln:%s", err.Error()) | ||
| } | ||
|
|
||
| return Number{ | ||
| value: res, | ||
| } | ||
| } |
There was a problem hiding this comment.
Add check for logarithm of non-positive numbers
The Ln method needs to check if the input is zero or negative before attempting to calculate the natural logarithm, as logarithms are only defined for positive numbers.
func (n Number) Ln(precision int32) Value {
+ if n.value.IsZero() || n.value.IsNegative() {
+ return NewUndefinedWithReasonf("logarithm of non-positive number")
+ }
res, err := n.value.Ln(precision)
if err != nil {
return NewUndefinedWithReasonf("Ln:%s", err.Error())
}
return Number{
value: res,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (n Number) Ln(precision int32) Value { | |
| res, err := n.value.Ln(precision) | |
| if err != nil { | |
| return NewUndefinedWithReasonf("Ln:%s", err.Error()) | |
| } | |
| return Number{ | |
| value: res, | |
| } | |
| } | |
| func (n Number) Ln(precision int32) Value { | |
| if n.value.IsZero() || n.value.IsNegative() { | |
| return NewUndefinedWithReasonf("logarithm of non-positive number") | |
| } | |
| res, err := n.value.Ln(precision) | |
| if err != nil { | |
| return NewUndefinedWithReasonf("Ln:%s", err.Error()) | |
| } | |
| return Number{ | |
| value: res, | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
README.md (1)
234-234: Hard tab still present in code block – markdown-lint MD010A previous review already requested replacing leading tabs with spaces for consistent formatting; the issue remains on this line.
Please convert the tab in front ofexpr := …to four spaces.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
234-234: Hard tabs
Column: 1(MD010, no-hard-tabs)
value.go (2)
5-33: Interface still excessively large – prior feedback not addressedThe
Valueinterface continues to oblige every concrete type to implement the full arithmetic + logical surface, even when some operations make no semantic sense (e.g.AddonBool). Earlier advice suggested splitting this into focused, composable interfaces (Arithmetic,Comparable,Logical, …).Keeping the monolithic interface increases implementation burden and hinders future evolution.
Consider factoring it as previously outlined.
64-70:ToNumberstill returns a pseudo-Number that can hide errorsReturning a
Numberwith an embeddedUndefinedrisks callers treating it as a valid numeric value. Earlier feedback proposed changing the signature toValue(or introducing aMaybeNumber) so undefined cases cannot be ignored.The same applies to
ToBool(lines 76-82).value_number.go (6)
79-87:⚠️ Potential issueDivision by zero still unguarded
decimal.Decimal.Divpanics on a zero divisor. A check should be added before performing the division.- return Number{ - value: n.value.Div(v.Number().value), - } + if v.Number().value.IsZero() { + return NewUndefinedWithReasonf("division by zero") + } + return Number{ + value: n.value.Div(v.Number().value), + }
89-97:⚠️ Potential issueZero raised to a negative power is undefined
Guard this edge case to avoid invalid values or panics.
- return Number{ - value: n.value.Pow(v.Number().value), - } + if n.value.IsZero() && v.Number().value.IsNegative() { + return NewUndefinedWithReasonf("cannot raise zero to a negative power") + } + return Number{ + value: n.value.Pow(v.Number().value), + }
99-107:⚠️ Potential issueModulo by zero not handled
Add the usual zero-divisor guard to prevent panics.
- return Number{ - value: n.value.Mod(v.Number().value), - } + if v.Number().value.IsZero() { + return NewUndefinedWithReasonf("modulo by zero") + } + return Number{ + value: n.value.Mod(v.Number().value), + }
167-176:⚠️ Potential issue
Sqrtaccepts negative numbers – returns NaN
sqrt(-n)yieldsNaNfrombig.Floatwhich is later parsed as an error. Provide an explicit negativity check and a clear error message.- n, err := NewNumberFromString( - new(big.Float).Sqrt(n.value.BigFloat()).String(), - ) - if err != nil { - return NewUndefinedWithReasonf("Sqrt:%s", err.Error()) - } + if n.value.IsNegative() { + return NewUndefinedWithReasonf("square root of negative number") + } + n, err := NewNumberFromString( + new(big.Float).Sqrt(n.value.BigFloat()).String(), + ) + if err != nil { + return NewUndefinedWithReasonf("Sqrt:%s", err.Error()) + }
184-193:⚠️ Potential issue
Lnshould reject non-positive inputsNatural logarithm is undefined for zero or negative numbers – return
Undefinedinstead of delegating toLnwhich will error.- res, err := n.value.Ln(precision) + if n.value.IsZero() || n.value.IsNegative() { + return NewUndefinedWithReasonf("logarithm of non-positive number") + } + res, err := n.value.Ln(precision)
195-209:⚠️ Potential issue
Logbase-10 likewise needs non-positive guardSame issue as
Ln, plus you can skip the expensive calculation when the argument is invalid.- res, err := n.value.Ln(precision + 1) + if n.value.IsZero() || n.value.IsNegative() { + return NewUndefinedWithReasonf("logarithm of non-positive number") + } + res, err := n.value.Ln(precision + 1)
🧹 Nitpick comments (1)
README.md (1)
225-231: Typo and minor grammar fix – “porperties”There is a typo in the new “Objects Dot accessors” section: porperties → properties.
Consider also re-phrasing the first sentence for clarity.-While user-defined Objects are generally Value-centric, `gal` supports accessing porperties and methods on Go objects too, using the `.` accessor. +While user-defined Objects are generally Value-centric, `gal` also supports accessing properties and methods on plain Go objects with the `.` accessor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
static/bit.ly_3MDZ9QT.pngis excluded by!**/*.png
📒 Files selected for processing (12)
README.md(3 hunks)gal.go(0 hunks)object.go(2 hunks)tree.go(2 hunks)tree_builder.go(4 hunks)tree_config.go(1 hunks)value.go(3 hunks)value_bool.go(1 hunks)value_multivalue.go(1 hunks)value_number.go(1 hunks)value_string.go(1 hunks)value_undefined.go(1 hunks)
💤 Files with no reviewable changes (1)
- gal.go
✅ Files skipped from review due to trivial changes (1)
- tree_builder.go
🚧 Files skipped from review as they are similar to previous changes (7)
- object.go
- value_multivalue.go
- tree.go
- value_bool.go
- value_string.go
- tree_config.go
- value_undefined.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
value.go (5)
operator.go (12)
Multiply(17-17)Divide(18-18)LShift(21-21)RShift(22-22)LessThan(23-23)LessThanOrEqual(24-24)EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)And(29-29)Or(31-31)value_bool.go (2)
Bool(5-8)Bool(27-29)value_number.go (2)
Number(10-13)Number(42-44)value_undefined.go (13)
Undefined(16-18)Undefined(30-32)Undefined(63-65)Undefined(67-69)Undefined(71-73)Undefined(75-77)Undefined(79-81)Undefined(83-85)Undefined(87-89)Undefined(91-93)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)value_string.go (2)
String(5-8)String(14-16)
value_number.go (6)
value_undefined.go (13)
Undefined(16-18)Undefined(30-32)Undefined(63-65)Undefined(67-69)Undefined(71-73)Undefined(75-77)Undefined(79-81)Undefined(83-85)Undefined(87-89)Undefined(91-93)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)value.go (2)
Value(5-33)Numberer(39-41)value_string.go (3)
String(5-8)String(14-16)NewString(10-12)operator.go (10)
Multiply(17-17)Divide(18-18)LShift(21-21)RShift(22-22)LessThanOrEqual(24-24)LessThan(23-23)EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)function.go (9)
Floor(222-233)Sin(150-161)Cos(136-147)Sqrt(208-219)Tan(164-175)Ln(178-190)Log(193-205)Trunc(235-253)Factorial(124-133)value_bool.go (5)
Bool(5-8)Bool(27-29)NewBool(10-12)False(88-88)True(89-89)
🪛 markdownlint-cli2 (0.17.2)
README.md
234-234: Hard tabs
Column: 1
(MD010, no-hard-tabs)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
README.md (1)
234-245: Replace hard tabs with spaces inside fenced code blocks.
markdownlint(MD010) still reports a hard-tab at line 234. The same issue was raised previously and remains unresolved.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
234-234: Hard tabs
Column: 1(MD010, no-hard-tabs)
value.go (2)
64-70:ToNumbersilently wraps a failure inside aNumberthat still satisfiesNumberer.
Callers may treat the returned value as a valid number and propagate an undefined state. Previous review suggested returningValue(orUndefined) instead.
5-33: 🛠️ Refactor suggestion
Valueinterface is still monolithic – earlier slim-down advice remains unaddressed.
The interface obliges every concrete type to satisfy ~20 methods, many of which are irrelevant (e.g. arithmetic onBool). This was flagged in a prior review and the concern still stands.value_string.go (3)
76-80:⚠️ Potential issueUnchecked repeat count can panic and overflow.
strings.Repeatwill panic on negative or huge counts, and theint64→intcast may overflow on 32-bit builds. This was highlighted earlier but not fixed.if v, ok := other.(Numberer); ok { - return String{ - value: strings.Repeat(s.value, int(v.Number().value.IntPart())), - } + n := v.Number().value + if !n.IsInteger() || n.IsNegative() { + return NewUndefinedWithReasonf("invalid repeat count: %s", n) + } + count := n.IntPart() + if int64(int(count)) != count { // overflow guard + return NewUndefinedWithReasonf("repeat count overflows on this architecture") + } + return String{value: strings.Repeat(s.value, int(count))} }
86-101:⚠️ Potential issueSlice index uses
int64→ compilation error & missing bounds checks.
stringslicing requiresint. UsingIntPart()(int64) will not compile and may overflow/bounds-panic. Same feedback was given before.- return String{ - value: s.value[v.Number().value.IntPart():], - } + idx := v.Number().value.IntPart() + if idx < 0 || idx > int64(len(s.value)) { + return NewUndefinedWithReasonf("left shift out of range") + } + return String{value: s.value[int(idx):]}
104-118:⚠️ Potential issueRight-shift repeats the same compile-time & runtime issues and mislabels errors.
The function still:
• usesint64indices,
• lacks bounds checks,
• reports “left shift” in error messages.- if v.Number().value.IsNegative() { - return NewUndefinedWithReasonf("invalid negative left shift") + if v.Number().value.IsNegative() { + return NewUndefinedWithReasonf("invalid negative right shift") } - if !v.Number().value.IsInteger() { - return NewUndefinedWithReasonf("invalid non-integer left shift") + if !v.Number().value.IsInteger() { + return NewUndefinedWithReasonf("invalid non-integer right shift") } - - return String{ - value: s.value[:int64(len(s.value))-v.Number().value.IntPart()], - } + shift := v.Number().value.IntPart() + limit := int64(len(s.value)) - shift + if shift < 0 || limit < 0 { + return NewUndefinedWithReasonf("right shift out of range") + } + return String{value: s.value[:int(limit)]}
🧹 Nitpick comments (2)
README.md (1)
225-225: Correct the typo “porperties” → “properties”.
Small spelling slip in the newly-added paragraph.value.go (1)
76-82:ToBoolmirrors the same silent-failure pattern asToNumber.
If the cast fails it fabricates aBoolwith an undefined flag butfalsevalue, which can be mistaken for a legitimate boolean. Consider returningValueor an explicit error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
static/bit.ly_3MDZ9QT.pngis excluded by!**/*.png
📒 Files selected for processing (13)
README.md(3 hunks)gal.go(0 hunks)gal_test.go(1 hunks)object.go(2 hunks)tree.go(2 hunks)tree_builder.go(4 hunks)tree_config.go(1 hunks)value.go(3 hunks)value_bool.go(1 hunks)value_multivalue.go(1 hunks)value_number.go(1 hunks)value_string.go(1 hunks)value_undefined.go(1 hunks)
💤 Files with no reviewable changes (1)
- gal.go
✅ Files skipped from review due to trivial changes (2)
- object.go
- gal_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- tree_builder.go
- value_multivalue.go
- tree.go
- tree_config.go
- value_bool.go
- value_undefined.go
- value_number.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
value_string.go (7)
value_undefined.go (13)
Undefined(16-18)Undefined(30-32)Undefined(63-65)Undefined(67-69)Undefined(71-73)Undefined(75-77)Undefined(79-81)Undefined(83-85)Undefined(87-89)Undefined(91-93)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)operator.go (9)
LessThan(23-23)LessThanOrEqual(24-24)EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)Multiply(17-17)LShift(21-21)RShift(22-22)value.go (3)
Value(5-33)Stringer(35-37)Numberer(39-41)value_bool.go (4)
Bool(5-8)Bool(27-29)NewBool(10-12)False(88-88)value_number.go (3)
Number(10-13)Number(42-44)NewNumberFromString(33-40)function.go (1)
Eval(255-267)tree_builder.go (1)
NewTreeBuilder(11-13)
value.go (4)
value_bool.go (2)
Bool(5-8)Bool(27-29)value_number.go (2)
Number(10-13)Number(42-44)value_undefined.go (13)
Undefined(16-18)Undefined(30-32)Undefined(63-65)Undefined(67-69)Undefined(71-73)Undefined(75-77)Undefined(79-81)Undefined(83-85)Undefined(87-89)Undefined(91-93)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)value_string.go (2)
String(5-8)String(14-16)
🪛 markdownlint-cli2 (0.17.2)
README.md
234-234: Hard tabs
Column: 1
(MD010, no-hard-tabs)
seborama
force-pushed
the
split_files
branch
4 times, most recently
from
2fb594f to
a677c18
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
README.md (1)
234-241: Hard tabs still present – please convert to spaces.
markdownlintcontinues to flag MD010 on these lines. A previous review already highlighted this; the issue is still unresolved.Replacing each leading tab with four spaces will restore consistent formatting across the code examples.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
234-234: Hard tabs
Column: 1(MD010, no-hard-tabs)
value_multivalue.go (1)
56-61:⚠️ Potential issueNegative-index access can panic.
Getonly checksi >= len(m.values); a negativeiwill still trigger an out-of-range panic.
Guard the lower bound as well:-func (m MultiValue) Get(i int) Value { - if i >= len(m.values) { +func (m MultiValue) Get(i int) Value { + if i < 0 || i >= len(m.values) { return NewUndefinedWithReasonf( "out of bounds: trying to get arg #%d on MultiValue that has %d arguments", i, len(m.values)) }value.go (1)
73-79: 🛠️ Refactor suggestionReturning a sentinel
Numberstill hides error states.Previous feedback suggested returning
Value(orUndefined) directly so callers must handle the conversion failure. By returning aNumberthat masquerades asvalueEntryKind, downstream arithmetic can silently propagate the error.Re-consider adapting the earlier recommendation:
-func ToNumber(val Value) Number { +func ToNumber(val Value) Value {value_bool.go (1)
51-56: 🛠️ Refactor suggestionImprove error handling when logical operations have non-Bool operands
When
otheris not aBooler, theAndmethod silently returnsFalse. This masks programming errors -True.And(42)becomesFalsewith no hint something went wrong.func (b Bool) And(other Value) Bool { if v, ok := other.(Booler); ok { return NewBool(b.value && v.Bool().value) } - return False // NOTE: should Bool be a Maybe? + return Bool{Undefined: NewUndefinedWithReasonf("logical And requires Bool, got %T", other)} }value_undefined.go (1)
117-119:⚠️ Potential issueIncorrect implementation of IsUndefined()
The implementation of
IsUndefined()is inaccurate, as noted in the comment. AnUndefinedvalue is always undefined regardless of whether it has a reason.func (u Undefined) IsUndefined() bool { - return u.reason == "" // NOTE: this is not quite accurate: an Undefined may not hold a reason + return true // An Undefined is always undefined }
🧹 Nitpick comments (4)
README.md (1)
223-231: Fix typo “porperties” → “properties”.Minor spelling mistake in the newly-added heading; correcting it helps keep the documentation professional.
-While user-defined Objects are generally Value-centric, `gal` supports accessing porperties and methods on Go objects too, using the `.` accessor. +While user-defined Objects are generally Value-centric, `gal` supports accessing properties and methods on Go objects too, using the `.` accessor.value_multivalue.go (1)
34-39: Undefined comparison path needs a unit test.The TODO notes an intent to test the equality logic. Consider adding a case where one of the embedded values is
Undefined; this will confirm the chosen semantics (Undefined == Undefined → ?).If you want help scaffolding the test, let me know.
value_bool.go (1)
16-25: Consider returning Undefined on string conversion failureThe
NewBoolFromStringfunction returnsFalsewith an error when conversion fails. This can lead to bugs if callers don't check the error. Consider using the alternative approach mentioned in the comment.func NewBoolFromString(s string) (Bool, error) { switch s { case "True": return True, nil case "False": return False, nil default: - return False, errors.Errorf("'%s' cannot be converted to a Bool", s) + return Bool{Undefined: NewUndefinedWithReasonf("cannot convert '%s' to Bool", s)}, + errors.Errorf("'%s' cannot be converted to a Bool", s) } }value_undefined.go (1)
95-97: Improve error messages for logical operations with UndefinedThe error messages could be made clearer about the operation being performed.
func (Undefined) And(other Value) Bool { - return Bool{Undefined: NewUndefinedWithReasonf("error: '%T/%s':'%s' cannot use And with Undefined", other, other.kind().String(), other.String())} + return Bool{Undefined: NewUndefinedWithReasonf("logical And error: cannot combine Undefined with '%T/%s':'%s'", other, other.kind().String(), other.String())} } func (Undefined) Or(other Value) Bool { - return Bool{Undefined: NewUndefinedWithReasonf("error: '%T/%s':'%s' cannot use Or with Undefined", other, other.kind().String(), other.String())} + return Bool{Undefined: NewUndefinedWithReasonf("logical Or error: cannot combine Undefined with '%T/%s':'%s'", other, other.kind().String(), other.String())} }Also applies to: 99-101
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
static/bit.ly_3MDZ9QT.pngis excluded by!**/*.png
📒 Files selected for processing (13)
README.md(3 hunks)gal.go(0 hunks)gal_test.go(1 hunks)object.go(2 hunks)tree.go(2 hunks)tree_builder.go(4 hunks)tree_config.go(1 hunks)value.go(3 hunks)value_bool.go(1 hunks)value_multivalue.go(1 hunks)value_number.go(1 hunks)value_string.go(1 hunks)value_undefined.go(1 hunks)
💤 Files with no reviewable changes (1)
- gal.go
✅ Files skipped from review due to trivial changes (1)
- object.go
🚧 Files skipped from review as they are similar to previous changes (5)
- tree_builder.go
- gal_test.go
- tree.go
- tree_config.go
- value_number.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
value_multivalue.go (5)
value_undefined.go (5)
Undefined(16-18)Undefined(30-32)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)value.go (1)
Value(5-9)operator.go (1)
EqualTo(25-25)value_bool.go (1)
False(88-88)value_string.go (3)
String(5-8)String(14-16)NewString(10-12)
value_string.go (6)
value_undefined.go (5)
Undefined(16-18)Undefined(30-32)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)value.go (3)
Value(5-9)Stringer(44-46)Numberer(48-50)value_bool.go (4)
Bool(5-8)Bool(27-29)NewBool(10-12)False(88-88)value_number.go (3)
Number(10-13)Number(42-44)NewNumberFromString(33-40)function.go (1)
Eval(255-267)tree_builder.go (1)
NewTreeBuilder(11-13)
value_undefined.go (4)
operator.go (12)
EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)LessThan(23-23)LessThanOrEqual(24-24)Multiply(17-17)Divide(18-18)LShift(21-21)RShift(22-22)And(29-29)Or(31-31)value.go (1)
Value(5-9)value_bool.go (4)
Bool(5-8)Bool(27-29)False(88-88)True(89-89)value_string.go (3)
String(5-8)String(14-16)NewString(10-12)
🪛 GitHub Actions: Test and Build
value_string.go
[error] 93-93: if ok has complex nested blocks (complexity: 4) (nestif)
[error] 116-116: if ok has complex nested blocks (complexity: 4) (nestif)
🪛 markdownlint-cli2 (0.17.2)
README.md
234-234: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (2)
value_undefined.go (2)
10-15: Excellent documentation of the Undefined typeThe documentation clearly explains the purpose of the Undefined type and how it's used within the codebase, including the pattern of embedding it in other value types to provide default behavior.
63-65: Good implementation of propagating undefined stateAll arithmetic and bitwise operations now properly return the original undefined value (including its reason) instead of a new empty undefined value. This ensures error context is preserved when operations are chained.
Also applies to: 67-69, 71-73, 75-77, 79-81, 83-85, 87-89, 91-93
| func (b Bool) Or(other Value) Bool { | ||
| if v, ok := other.(Booler); ok { | ||
| return NewBool(b.value || v.Bool().value) | ||
| } | ||
| return False // NOTE: should Bool be a Maybe? | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling when logical operations have non-Bool operands
Similar to the And method, the Or method silently returns False for non-Boolean operands.
func (b Bool) Or(other Value) Bool {
if v, ok := other.(Booler); ok {
return NewBool(b.value || v.Bool().value)
}
- return False // NOTE: should Bool be a Maybe?
+ return Bool{Undefined: NewUndefinedWithReasonf("logical Or requires Bool, got %T", other)}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (b Bool) Or(other Value) Bool { | |
| if v, ok := other.(Booler); ok { | |
| return NewBool(b.value || v.Bool().value) | |
| } | |
| return False // NOTE: should Bool be a Maybe? | |
| } | |
| func (b Bool) Or(other Value) Bool { | |
| if v, ok := other.(Booler); ok { | |
| return NewBool(b.value || v.Bool().value) | |
| } | |
| return Bool{Undefined: NewUndefinedWithReasonf("logical Or requires Bool, got %T", other)} | |
| } |
| // TODO: add test to confirm this is correct! | ||
| func (s String) RShift(other Value) Value { |
There was a problem hiding this comment.
Reduce complexity in RShift and fix redundant computation
Similar to LShift, we can reduce complexity by using the validateShift function. There's also a redundant computation of the string length.
// TODO: add test to confirm this is correct!
func (s String) RShift(other Value) Value {
- if v, ok := other.(Numberer); ok {
- if v.Number().value.IsNegative() {
- return NewUndefinedWithReasonf("invalid negative right shift")
- }
- if !v.Number().value.IsInteger() {
- return NewUndefinedWithReasonf("invalid non-integer right shift")
- }
-
- shift := v.Number().value.IntPart()
- if shift < 0 {
- return NewUndefinedWithReasonf("right shift [%s]: out of range", other.String())
- }
- limit := int64(len(s.value))
- if shift > limit {
- return String{}
- }
-
- return String{
- value: s.value[:int64(len(s.value))-v.Number().value.IntPart()],
- }
+ shift, undef := validateShift(other, false)
+ if undef.reason != "" {
+ return undef
}
-
- return NewUndefinedWithReasonf("NaN: %s", other.String())
+
+ limit := int64(len(s.value))
+ if shift > limit {
+ return String{}
+ }
+
+ return String{value: s.value[:int(limit-shift)]}
}Also applies to: 116-139
| // TODO: add test to confirm this is correct! | ||
| func (s String) LShift(other Value) Value { |
There was a problem hiding this comment.
Reduce complexity in LShift to address linter errors
The linter is reporting complex nested blocks. We can simplify by extracting checks into a separate function.
+// validateShift checks that the shift value is a non-negative integer and returns an error message if not
+func validateShift(val Value, isLeft bool) (int64, Undefined) {
+ shiftType := "left"
+ if !isLeft {
+ shiftType = "right"
+ }
+
+ v, ok := val.(Numberer)
+ if !ok {
+ return 0, NewUndefinedWithReasonf("NaN: %s", val.String())
+ }
+
+ num := v.Number().value
+ if num.IsNegative() {
+ return 0, NewUndefinedWithReasonf("invalid negative %s shift", shiftType)
+ }
+ if !num.IsInteger() {
+ return 0, NewUndefinedWithReasonf("invalid non-integer %s shift", shiftType)
+ }
+
+ idx := num.IntPart()
+ if idx < 0 {
+ return 0, NewUndefinedWithReasonf("%s shift [%s]: out of range", shiftType, val.String())
+ }
+
+ return idx, Undefined{}
+}
+
// TODO: add test to confirm this is correct!
func (s String) LShift(other Value) Value {
- if v, ok := other.(Numberer); ok {
- if v.Number().value.IsNegative() {
- return NewUndefinedWithReasonf("invalid negative left shift")
- }
- if !v.Number().value.IsInteger() {
- return NewUndefinedWithReasonf("invalid non-integer left shift")
- }
-
- idx64 := v.Number().value.IntPart()
- if idx64 < 0 {
- return NewUndefinedWithReasonf("left shift [%s]: out of range", other.String())
- }
- if idx64 > int64(len(s.value)) {
- return String{}
- }
- return String{value: s.value[int(idx64):]}
+ idx64, undef := validateShift(other, true)
+ if undef.reason != "" {
+ return undef
}
-
- return NewUndefinedWithReasonf("NaN: %s", other.String())
+
+ if idx64 > int64(len(s.value)) {
+ return String{}
+ }
+ return String{value: s.value[int(idx64):]}
}Also applies to: 93-112
| return NewUndefinedWithReasonf("NaN: %s", other.String()) | ||
| } | ||
|
|
||
| // TODO: add test to confirm this is correct! |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add tests for substring operations as indicated by TODOs
The TODO comments indicate that tests are needed to confirm the correctness of LShift and RShift operations.
Also applies to: 114-114
🏁 Script executed:
#!/bin/bash
# Check if there are tests for LShift and RShift operations
echo "Searching for tests of String.LShift and String.RShift..."
rg -i 'test.*(?:lshift|rshift|shift).*string' --type goLength of output: 182
🏁 Script executed:
#!/bin/bash
set -e
# Locate value_string.go
file=$(find . -type f -name value_string.go | head -n1)
if [[ -z "$file" ]]; then
echo "value_string.go not found"
exit 1
fi
echo "Found file: $file"
# Show lines around the TODO
echo "--- value_string.go (lines 70–100) ---"
sed -n '70,100p' "$file"
# Search for LShift references in test files
echo "--- Searching for LShift in *_test.go files ---"
rg 'LShift' -g '*_test.go' -n || echo "No LShift test references found"
# Search for RShift references in test files
echo "--- Searching for RShift in *_test.go files ---"
rg 'RShift' -g '*_test.go' -n || echo "No RShift test references found"Length of output: 1687
Add missing unit tests for String.LShift and String.RShift
The two TODOs in value_string.go indicate that neither the left-shift nor right-shift string operations are covered by tests. Our search across all *_test.go files returned no references to either LShift or RShift.
Please add unit tests for:
- value_string.go, line 91:
func (s String) LShift(other Value) Value { … } - value_string.go, line 114:
func (s String) RShift(other Value) Value { … }
Ensure the tests validate both normal and error cases (negative or non-integer shifts) as per the implementation’s undefined-value logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
value_number_test.go (1)
10-993: 💡 Verification agent❓ Verification inconclusive
All test functions contain only TODO stubs – they provide zero coverage
The file declares ~40 test functions but each table is empty. This creates the illusion of test coverage while exercising none of the behaviour you painstakingly implemented.
Either:
- Populate the tables with meaningful cases, or
- Remove the stubs until real tests are ready – empty tests add maintenance overhead and slow down
go testfor no benefit.Failing to address this will likely mask regressions in the numeric stack.
Do you want help generating a focused set of table-driven tests for the critical operations?
Empty test stubs in value_number_test.go – zero coverage
All 40+ table-driven tests in
value_number_test.gocontain no test cases, so none of yourNumberbehaviour is actually exercised. This not only adds maintenance overhead but also hides regressions in the numeric stack.Please address this by either:
- Populating each
testsslice with meaningful cases for critical methods (e.g. constructors, arithmetic, comparisons, conversions), or- Removing the stub functions until real test cases are ready.
Location:
- File:
value_number_test.go(lines 10–993)Let me know if you’d like example table-driven tests for key operations.
♻️ Duplicate comments (4)
value_number.go (4)
90-98:⚠️ Potential issueZero to a negative power is still not guarded
This edge-case was raised in an earlier review but the check is still missing.decimal.Powwill return an infinity-like value which later operations may treat as valid.if v, ok := other.(Numberer); ok { + // 0 ** (−ve) is mathematically undefined (division by zero) + if n.value.IsZero() && v.Number().value.IsNegative() { + return NewUndefinedWithReasonf("cannot raise zero to a negative power") + } return Number{ value: n.value.Pow(v.Number().value), } }
100-108: 🛠️ Refactor suggestionReturn a clearer error for ‘modulo by zero’
The current branch silently falls through to the generic “NaN” error when the divisor is zero. Surfacing a specific message greatly helps users diagnose the problem.
-if v, ok := other.(Numberer); ok && !v.Number().value.IsZero() { - return Number{ value: n.value.Mod(v.Number().value) } +if v, ok := other.(Numberer); ok { + if v.Number().value.IsZero() { + return NewUndefinedWithReasonf("modulo by zero") + } + return Number{ value: n.value.Mod(v.Number().value) } }
188-197:⚠️ Potential issueMissing non-positive check in
LnComputing
Lnfor zero or negative numbers is undefined. Relying on the underlying library to error is fragile and gives a less descriptive message.-func (n Number) Ln(precision int32) Value { - res, err := n.value.Ln(precision) +func (n Number) Ln(precision int32) Value { + if n.value.IsZero() || n.value.IsNegative() { + return NewUndefinedWithReasonf("logarithm of non-positive number") + } + res, err := n.value.Ln(precision)
199-213:⚠️ Potential issueSame non-positive guard needed in
Log
Logdelegates toLn, so the same pre-condition applies. An early guard avoids double computation and propagates a clearer reason.-func (n Number) Log(precision int32) Value { +func (n Number) Log(precision int32) Value { + if n.value.IsZero() || n.value.IsNegative() { + return NewUndefinedWithReasonf("logarithm of non-positive number") + } res, err := n.value.Ln(precision + 1)
🧹 Nitpick comments (3)
value.go (2)
38-42: Consider renamingStringerto avoid confusion withfmt.Stringer
valueHelperembeds both your ownStringerandfmt.Stringer. Although the two expose different methods (AsString()vsString()), the very similar names make it easy to mistake one for the other while reading or implementing types.A small rename (e.g.
StringConvertible) would clarify intent and avoid future mix-ups.
68-71: Questionable benefit of theokreturn flag intoValue
toValuereturns a boolean but every call site in the file immediately discards it, relying on the fact that anUndefinedwith a reason is still a validValue. Unless other packages consume the flag, consider dropping it to simplify the API.Diff sketch:
-func toValue(value any) (Value, bool) { +func toValue(value any) Value { v, err := goAnyToGalType(value) if err != nil { - return NewUndefinedWithReasonf("value type %T - %s", value, err.Error()), false + return NewUndefinedWithReasonf("value type %T - %s", value, err.Error()) } - return v, true + return v }value_number.go (1)
116-131: Repeated validation logic in shift operations – consider factoring out
LShiftandRShiftshare identical checks for negative and non-integer shift counts. Extracting a small helper would reduce duplication and the risk of diverging behaviour.Example:
func validateShift(v Number) Undefined { switch { case v.value.IsNegative(): return NewUndefinedWithReasonf("invalid negative shift") case !v.value.IsInteger(): return NewUndefinedWithReasonf("invalid non-integer shift") } return Undefined{} }Use the helper in both methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(3 hunks)value.go(3 hunks)value_number.go(1 hunks)value_number_test.go(1 hunks)value_string.go(1 hunks)value_undefined.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- value_undefined.go
- value_string.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
value.go (5)
operator.go (12)
Multiply(17-17)Divide(18-18)LShift(21-21)RShift(22-22)LessThan(23-23)LessThanOrEqual(24-24)EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)And(29-29)Or(31-31)value_bool.go (2)
Bool(5-8)Bool(27-29)value_number.go (2)
Number(10-13)Number(42-44)value_undefined.go (5)
Undefined(16-18)Undefined(30-32)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)value_string.go (2)
String(5-8)String(14-16)
value_number.go (6)
value_undefined.go (5)
Undefined(16-18)Undefined(30-32)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)value.go (2)
Value(5-11)Numberer(56-58)value_string.go (3)
String(5-8)String(14-16)NewString(10-12)operator.go (10)
Multiply(17-17)Divide(18-18)LShift(21-21)RShift(22-22)LessThanOrEqual(24-24)LessThan(23-23)EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)function.go (9)
Floor(222-233)Sin(150-161)Cos(136-147)Sqrt(208-219)Tan(164-175)Ln(178-190)Log(193-205)Trunc(235-253)Factorial(124-133)value_bool.go (5)
Bool(5-8)Bool(27-29)NewBool(10-12)False(88-88)True(89-89)
value_number_test.go (5)
value_number.go (6)
Number(10-13)Number(42-44)NewNumber(15-19)NewNumberFromInt(21-25)NewNumberFromFloat(27-31)NewNumberFromString(33-40)value_undefined.go (4)
Undefined(16-18)Undefined(30-32)Undefined(95-97)Undefined(99-101)value.go (1)
Value(5-11)operator.go (10)
Multiply(17-17)Divide(18-18)LShift(21-21)RShift(22-22)LessThan(23-23)LessThanOrEqual(24-24)EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)function.go (8)
Sin(150-161)Cos(136-147)Tan(164-175)Ln(178-190)Log(193-205)Floor(222-233)Trunc(235-253)Factorial(124-133)
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
value_undefined.go (1)
117-119:⚠️ Potential issue
IsUndefinedstill reports false negativesThis point was raised in an earlier review: every
Undefinedis, by definition, undefined irrespective of thereasonfield. As long as the method relies on an empty reason string, valid undefined values with a reason remain invisible to callers.-func (u Undefined) IsUndefined() bool { - return u.reason == "" // NOTE: this is not quite accurate: an Undefined may not hold a reason -} +func (u Undefined) IsUndefined() bool { + return true +}Failing to fix this continues to propagate incorrect state checks throughout the evaluator.
value_number.go (3)
188-197:⚠️ Potential issueAdd check for logarithm of non-positive numbers
The
Lnmethod needs to check if the input is zero or negative before attempting to calculate the natural logarithm, as logarithms are only defined for positive numbers.func (n Number) Ln(precision int32) Value { + if n.value.IsZero() || n.value.IsNegative() { + return NewUndefinedWithReasonf("logarithm of non-positive number") + } res, err := n.value.Ln(precision) if err != nil { return NewUndefinedWithReasonf("Ln:%s", err.Error()) } return Number{ value: res, } }
199-213:⚠️ Potential issueAdd base 10 check in
LogimplementationThe
Logimplementation calculates the log base 10 by dividing the natural log by the natural log of 10. However, if the input is 0 or negative, then this will lead to an error. Add a check for this case:func (n Number) Log(precision int32) Value { + if n.value.IsZero() || n.value.IsNegative() { + return NewUndefinedWithReasonf("logarithm of non-positive number") + } res, err := n.value.Ln(precision + 1) if err != nil { return NewUndefinedWithReasonf("Log:%s", err.Error()) } res10, err := decimal.New(10, 0).Ln(precision + 1) if err != nil { return NewUndefinedWithReasonf("Log:%s", err.Error()) } return Number{ value: res.Div(res10).Truncate(precision), } }
90-98:⚠️ Potential issueAdd check for raising zero to negative power
The
PowerOfmethod needs to handle the mathematical undefined case of raising zero to a negative power, which would result in division by zero.func (n Number) PowerOf(other Value) Value { if v, ok := other.(Numberer); ok { + if n.value.IsZero() && v.Number().value.IsNegative() { + return NewUndefinedWithReasonf("cannot raise zero to a negative power") + } return Number{ value: n.value.Pow(v.Number().value), } } return NewUndefinedWithReasonf("NaN: %s", other.String()) }
🧹 Nitpick comments (2)
value_string.go (2)
91-104: Duplicate validation logic – extract a helper
LShiftandRShiftboth repeat the same negative / non-integer checks. Centralising this into avalidateShifthelper (as you already hinted at in earlier reviews) will de-duplicate code, reduce branching complexity and keep future fixes in one place.
75-86: Guard against extremely large repeat countsAlthough you detect
intoverflow, a very large but still representable count (e.g.1<<30on a 64-bit build) can allocate gigabytes of memory and crash the process.
Consider cappingcountto a reasonable maximum or returningUndefinedwhenlen(s.value)*countwould overflowint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(3 hunks)value.go(3 hunks)value_number.go(1 hunks)value_number_test.go(1 hunks)value_string.go(1 hunks)value_undefined.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧬 Code Graph Analysis (5)
value.go (5)
operator.go (12)
Multiply(17-17)Divide(18-18)LShift(21-21)RShift(22-22)LessThan(23-23)LessThanOrEqual(24-24)EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)And(29-29)Or(31-31)value_bool.go (2)
Bool(5-8)Bool(27-29)value_number.go (2)
Number(10-13)Number(42-44)value_undefined.go (5)
Undefined(16-18)Undefined(30-32)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)value_string.go (2)
String(5-8)String(14-16)
value_undefined.go (4)
operator.go (12)
EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)LessThan(23-23)LessThanOrEqual(24-24)Multiply(17-17)Divide(18-18)LShift(21-21)RShift(22-22)And(29-29)Or(31-31)value.go (1)
Value(5-11)value_bool.go (4)
Bool(5-8)Bool(27-29)False(88-88)True(89-89)value_string.go (3)
String(5-8)String(14-16)NewString(10-12)
value_string.go (7)
value_undefined.go (5)
Undefined(16-18)Undefined(30-32)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)operator.go (9)
LessThan(23-23)LessThanOrEqual(24-24)EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)Multiply(17-17)LShift(21-21)RShift(22-22)value.go (3)
Value(5-11)Stringer(52-54)Numberer(56-58)value_bool.go (4)
Bool(5-8)Bool(27-29)NewBool(10-12)False(88-88)value_number.go (3)
Number(10-13)Number(42-44)NewNumberFromString(33-40)function.go (1)
Eval(255-267)tree_builder.go (1)
NewTreeBuilder(11-13)
value_number.go (5)
value_undefined.go (5)
Undefined(16-18)Undefined(30-32)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)value.go (2)
Value(5-11)Numberer(56-58)value_string.go (3)
String(5-8)String(14-16)NewString(10-12)function.go (9)
Floor(222-233)Sin(150-161)Cos(136-147)Sqrt(208-219)Tan(164-175)Ln(178-190)Log(193-205)Trunc(235-253)Factorial(124-133)value_bool.go (5)
Bool(5-8)Bool(27-29)NewBool(10-12)False(88-88)True(89-89)
value_number_test.go (7)
value_number.go (6)
NewNumber(15-19)Number(10-13)Number(42-44)NewNumberFromInt(21-25)NewNumberFromFloat(27-31)NewNumberFromString(33-40)value_undefined.go (5)
Undefined(16-18)Undefined(30-32)Undefined(95-97)Undefined(99-101)NewUndefinedWithReasonf(24-28)value.go (1)
Value(5-11)value_multivalue.go (1)
NewMultiValue(19-21)value_string.go (2)
String(5-8)String(14-16)operator.go (10)
Multiply(17-17)Divide(18-18)LShift(21-21)RShift(22-22)LessThan(23-23)LessThanOrEqual(24-24)EqualTo(25-25)NotEqualTo(26-26)GreaterThan(27-27)GreaterThanOrEqual(28-28)function.go (8)
Sin(150-161)Cos(136-147)Tan(164-175)Ln(178-190)Log(193-205)Floor(222-233)Trunc(235-253)Factorial(124-133)
🔇 Additional comments (1)
value_number_test.go (1)
173-174: Implement missing test casesThe test file has a good structure covering all methods of the
Numbertype, but most test functions contain placeholder TODOs instead of actual test cases. Implementing comprehensive test cases is essential for ensuring the correctness of the implementation, especially for error handling and edge cases.Priority areas to implement first:
- Tests for error conditions like division by zero, logarithm of non-positive numbers, etc.
- Tests for edge cases like zero, negative numbers, very large/small numbers
- Tests for special mathematical properties (e.g., identity elements, commutative properties)
Let me know if you'd like help implementing these test cases.
Also applies to: 202-203, 231-232, 260-261, 289-290, 314-315, 343-344, 372-373, 397-398, 422-423, 447-448, 472-473, 497-498, 526-527, 555-556, 580-581, 609-610, 634-635, 663-664, 692-693, 721-722, 750-751, 779-780, 808-809, 833-834, 858-859, 883-884, 908-909, 933-934, 958-959
| func ToValue(value any) Value { | ||
| v, _ := toValue(value) | ||
| v, _ := toValue(value) // ignore "ok" because we are sure it is a valid Value, be it Undefined or not. | ||
| return v |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider propagating the ok flag returned by toValue
ToValue discards the boolean returned by toValue, silently converting any failure into an Undefined without giving the caller a chance to react. In practice this makes it impossible to distinguish between a genuine, well-formed value and a conversion failure.
-func ToValue(value any) Value {
- v, _ := toValue(value) // ignore "ok" because we are sure it is a valid Value, be it Undefined or not.
- return v
+func ToValue(value any) (Value, bool) {
+ return toValue(value)
}Exposing the success flag (or even the error) would make downstream code more predictable and testable.
🏁 Script executed:
#!/bin/bash
# Locate the definition of the unexported helper
rg -n "func toValue" -n
# Confirm the exported wrapper signature
rg -n "func ToValue" -n value.go
# Find all call sites of ToValue across the repo
rg -n "ToValue" -n .Length of output: 214
Avoid breaking the public API – provide a new TryToValue instead
Changing ToValue to return (Value, bool) will silently break any existing callers (we only see its definition in value.go and no internal call sites). To expose the conversion success flag without introducing a breaking change, add a new helper and deprecate the old one:
• File: value.go
• Around lines 68–74
Suggested diff:
func ToValue(value any) Value {
v, _ := toValue(value) // ignore "ok" as before
return v
}
+// TryToValue attempts to convert an `any` to Value, returning a boolean indicating success.
+func TryToValue(value any) (Value, bool) {
+ return toValue(value)
+}
// Deprecated: use TryToValue when you need to distinguish conversion failures.
+// ToValue remains for callers who expect a Value (defaulting to Undefined).This preserves backward compatibility while making the success flag available for new code.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func ToValue(value any) Value { | |
| v, _ := toValue(value) | |
| v, _ := toValue(value) // ignore "ok" because we are sure it is a valid Value, be it Undefined or not. | |
| return v | |
| func ToValue(value any) Value { | |
| v, _ := toValue(value) // ignore "ok" as before | |
| return v | |
| } | |
| // TryToValue attempts to convert an `any` to Value, returning a boolean indicating success. | |
| func TryToValue(value any) (Value, bool) { | |
| return toValue(value) | |
| } | |
| // Deprecated: use TryToValue when you need to distinguish conversion failures. | |
| // ToValue remains for callers who expect a Value (defaulting to Undefined). |
| if idx64 > int64(len(s.value)) { | ||
| return String{} | ||
| } | ||
|
|
||
| return String{value: s.value[int(idx64):]} |
There was a problem hiding this comment.
Empty-string result is reported as undefined
Returning String{} when the shift index exceeds the length yields an empty String that embeds a zero-value Undefined.
Because IsUndefined() currently returns true for an Undefined with an empty reason, callers will erroneously believe the operation failed.
- if idx64 > int64(len(s.value)) {
- return String{}
- }
+ if idx64 > int64(len(s.value)) {
+ // produce a valid (defined) empty string
+ return NewString("")
+ }Please apply the same fix in RShift (lines 134-138).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if idx64 > int64(len(s.value)) { | |
| return String{} | |
| } | |
| return String{value: s.value[int(idx64):]} | |
| func (s String) LShift(other Value) Value { | |
| idx64, undef := validateShift(other, true) | |
| if undef.reason != "" { | |
| return undef | |
| } | |
| if idx64 > int64(len(s.value)) { | |
| // produce a valid (defined) empty string | |
| return NewString("") | |
| } | |
| return String{value: s.value[int(idx64):]} | |
| } |
| func TestNumber_PowerOf(t *testing.T) { | ||
| type fields struct { | ||
| Undefined Undefined | ||
| value decimal.Decimal | ||
| } | ||
| type args struct { | ||
| other Value | ||
| } | ||
| tests := []struct { | ||
| name string | ||
| fields fields | ||
| args args | ||
| want Value | ||
| }{ | ||
| // TODO: Add test cases. | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| n := Number{ | ||
| Undefined: tt.fields.Undefined, | ||
| value: tt.fields.value, | ||
| } | ||
| if got := n.PowerOf(tt.args.other); !reflect.DeepEqual(got, tt.want) { | ||
| t.Errorf("Number.PowerOf() = %v, want %v", got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement test cases for PowerOf edge cases
The PowerOf method needs thorough testing for edge cases such as raising zero to negative power, raising zero to zero, and raising any number to zero.
Add test cases to verify:
- Zero raised to positive power equals zero
- Zero raised to zero equals one (mathematical convention)
- Zero raised to negative power returns Undefined
- Any number raised to zero equals one
- One raised to any power equals one
| func TestNumber_Add(t *testing.T) { | ||
| type fields struct { | ||
| Undefined Undefined | ||
| value decimal.Decimal | ||
| } | ||
| type args struct { | ||
| other Value | ||
| } | ||
| tests := []struct { | ||
| name string | ||
| fields fields | ||
| args args | ||
| want Value | ||
| }{ | ||
| { | ||
| name: "add two positive numbers", | ||
| fields: fields{value: decimal.New(5, 0)}, | ||
| args: args{other: Number{value: decimal.New(3, 0)}}, | ||
| want: Number{value: decimal.New(8, 0)}, | ||
| }, | ||
| { | ||
| name: "add a positive and a negative number", | ||
| fields: fields{value: decimal.New(5, 0)}, | ||
| args: args{other: Number{value: decimal.New(-3, 0)}}, | ||
| want: Number{value: decimal.New(2, 0)}, | ||
| }, | ||
| { | ||
| name: "add two negative numbers", | ||
| fields: fields{value: decimal.New(-5, 0)}, | ||
| args: args{other: Number{value: decimal.New(-3, 0)}}, | ||
| want: Number{value: decimal.New(-8, 0)}, | ||
| }, | ||
| { | ||
| name: "add a number and non-Numberer", | ||
| fields: fields{value: decimal.New(5, 0)}, | ||
| args: args{other: NewMultiValue()}, | ||
| want: NewUndefinedWithReasonf("NaN: %s", MultiValue{}.String()), | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| n := Number{ | ||
| Undefined: tt.fields.Undefined, | ||
| value: tt.fields.value, | ||
| } | ||
| if got := n.Add(tt.args.other); !reflect.DeepEqual(got, tt.want) { | ||
| t.Errorf("Number.Add() = %v, want %v", got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add more test cases for basic arithmetic operations
While the Add method has some good test cases, there are other important scenarios to test, such as precision handling, overflow behavior, and identity properties (e.g., adding zero).
Consider expanding the test cases to cover:
- Addition with large exponents
- Addition with different precisions
- Addition with identity element (zero)
- Addition result precision
Similar test coverage should be implemented for other arithmetic operations like Sub, Multiply, Divide, etc.
| func TestNumber_Ln(t *testing.T) { | ||
| type fields struct { | ||
| Undefined Undefined | ||
| value decimal.Decimal | ||
| } | ||
| type args struct { | ||
| precision int32 | ||
| } | ||
| tests := []struct { | ||
| name string | ||
| fields fields | ||
| args args | ||
| want Value | ||
| }{ | ||
| // TODO: Add test cases. | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| n := Number{ | ||
| Undefined: tt.fields.Undefined, | ||
| value: tt.fields.value, | ||
| } | ||
| if got := n.Ln(tt.args.precision); !reflect.DeepEqual(got, tt.want) { | ||
| t.Errorf("Number.Ln() = %v, want %v", got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestNumber_Log(t *testing.T) { | ||
| type fields struct { | ||
| Undefined Undefined | ||
| value decimal.Decimal | ||
| } | ||
| type args struct { | ||
| precision int32 | ||
| } | ||
| tests := []struct { | ||
| name string | ||
| fields fields | ||
| args args | ||
| want Value | ||
| }{ | ||
| // TODO: Add test cases. | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| n := Number{ | ||
| Undefined: tt.fields.Undefined, | ||
| value: tt.fields.value, | ||
| } | ||
| if got := n.Log(tt.args.precision); !reflect.DeepEqual(got, tt.want) { | ||
| t.Errorf("Number.Log() = %v, want %v", got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests for logarithm edge cases
Both Ln and Log methods should have tests for edge cases, particularly around non-positive inputs which should return Undefined values.
Add test cases to verify:
- Logarithm of zero returns Undefined
- Logarithm of negative numbers returns Undefined
- Logarithm of one equals zero
- Logarithm of positive numbers returns correct values with proper precision
Summary by CodeRabbit
New Features
Documentation
Style