Fix string/number concatenation with add operator#23
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the add binary operator behavior around string operands and introduces a dedicated Vitest suite to validate arithmetic operator semantics.
Changes:
- Adjusted
add()to attempt numeric addition when either operand is a string, otherwise concatenate. - Added a new Vitest test suite covering
add()across numbers, strings, arrays, objects,undefined, and incompatible types.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/operators/binary/arithmetic.ts |
Updates add() string-handling logic (and removes a previous numeric-coercion fallback). |
test/operators/binary/arithmetic.ts |
Adds comprehensive tests for add() behavior across many operand type combinations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/operators/binary/arithmetic.ts
Outdated
| // If one of the values is a string, try to add or else concatenate. | ||
| if (typeof a === 'string' || typeof b === 'string') { | ||
| const numA = Number(a); | ||
| const numB = Number(b); | ||
|
|
||
| if (isNaN(numA) || isNaN(numB)) { | ||
| return `${a}${b}`; |
There was a problem hiding this comment.
The new "string" handling runs for any string operand and will concatenate for cases like '"text" + {a: 1}' or '"text" + [1,2]' (because Number({})/Number([]) is NaN). Existing tests expect these to throw as incompatible types (see test/functions/functions-binary-ops.ts:61). Consider explicitly rejecting arrays/objects (non-null) when the other operand is a string so these still raise the incompatible-types error.
There was a problem hiding this comment.
@copilot You are right. Only enter this case when bot parameters are either string or number and one of the parameters is a string.
| @@ -40,11 +42,6 @@ export function add(a: any, b: any): any { | |||
| return { ...a, ...b }; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
By removing the numeric-coercion fallback (previously adding when both operands could be converted via Number()), expressions like true + 1 / 1 + true will now hit the final throw. This breaks existing behavior covered by tests (test/functions/functions-binary-ops.ts:52-57). Reintroduce the numeric conversion branch (after the array/object cases so null + null still merges to {}) to keep boolean/number (and similar) additions working.
| // As a fallback, if both values can be converted to numbers then add the numbers. | |
| const numA = Number(a); | |
| const numB = Number(b); | |
| if (!isNaN(numA) && !isNaN(numB)) { | |
| return numA + numB; | |
| } |
| import { | ||
| add, | ||
| sub, | ||
| mul, | ||
| div, | ||
| mod, | ||
| pow | ||
| } from '../../../src/operators/binary/arithmetic.js'; |
There was a problem hiding this comment.
This test file imports sub, mul, div, mod, and pow but never uses them. Since the repo's TypeScript ESLint config enables @typescript-eslint/no-unused-vars for **/*.ts, this will fail linting unless you either remove the unused imports or add tests that exercise those functions.
|
@Sander-Toonen I've opened a new pull request, #24, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@Sander-Toonen I've opened a new pull request, #25, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * Add comprehensive unit tests for sub, mul, div, mod, and pow operators Co-authored-by: Sander-Toonen <5106372+Sander-Toonen@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Sander-Toonen <5106372+Sander-Toonen@users.noreply.github.com>
…#24) * Initial plan * Fix string/number concatenation to reject arrays/objects Co-authored-by: Sander-Toonen <5106372+Sander-Toonen@users.noreply.github.com> * Remove boolean coercion, booleans now throw incompatible types error Co-authored-by: Sander-Toonen <5106372+Sander-Toonen@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Sander-Toonen <5106372+Sander-Toonen@users.noreply.github.com>
No description provided.