Skip to content

Feature/take function - #1722

Open
Tobiadefami wants to merge 21 commits into
developfrom
feature/take-function
Open

Feature/take function#1722
Tobiadefami wants to merge 21 commits into
developfrom
feature/take-function

Conversation

@Tobiadefami

@Tobiadefami Tobiadefami commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Context

This PR adds the TAKE dynamic-array function. TAKE returns rows or columns from the beginning or end of an array and supports positive and negative counts, optional columns, syntactically empty argument slots, and array spilling.

A row or column count that truncates to zero returns the existing #N/A error with a TAKE-specific message. This is a documented Excel difference because HyperFormula does not expose a #CALC! error type. Omitting the required rows argument continues to return the existing wrong-argument #N/A error.

Implementation

  • Counts are truncated and clamped to the source dimensions; negative counts select from the end.
  • Address-backed sources remain lazy through sub-range spans.
  • Static spill-size prediction evaluates dependency-free literal percentages and arithmetic, so invalid counts and literal errors are resolved before spill allocation.
  • Whole-column sources spill from row 1 and return #SPILL! below row 1.
  • Function metadata, translations, changelog, and compatibility documentation are included.
  • The VSTACK/HSTACK localization fix now lives in Fix localized VSTACK and HSTACK names #1748 and is not part of this PR.

Validation

Companion tests: handsontable/hyperformula-tests#28

  • 69/69 focused TAKE tests passed.
  • 152/152 relevant metadata, localization, and optional-parameter tests passed.
  • The focused coverage run exercises invalid percentage sizing before spill allocation.
  • TypeScript compilation, targeted lint, and git diff --check passed.
  • Excel Online confirmed same-sheet and cross-sheet whole-column behavior and the row-1 spill boundary.

Types of changes

  • Breaking change
  • New feature or improvement
  • Bug fix
  • Additional language file or translation change
  • Documentation change

Checklist

  • The code follows the HyperFormula contribution guidelines and project style.
  • I have signed the Contributor License Agreement.
  • The change is compliant with OpenDocument 1.3.
  • The change is compatible with Microsoft Excel, subject to documented differences.
  • The change is compatible with Google Sheets.
  • The change is described in CHANGELOG.md.
  • Documentation has been updated.
  • A migration guide is required.

@Tobiadefami
Tobiadefami requested a review from sequba August 5, 2026 13:03
@netlify

netlify Bot commented Aug 5, 2026

Copy link
Copy Markdown

Deploy Preview for hyperformula-dev-docs ready!

Name Link
🔨 Latest commit b14d8ec
🔍 Latest deploy log https://app.netlify.com/projects/hyperformula-dev-docs/deploys/6a79f11bcfc9ef00085bcc14
😎 Deploy Preview https://deploy-preview-1722--hyperformula-dev-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 5, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
hyperformula-docs 3fd5a70 Commit Preview URL

Branch Preview URL
Aug 25 2026, 06:49 PM

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Performance comparison of head (3fd5a70) vs base (61ead73)

                                     testName |    base |    head |  change
---------------------------------------------------------------------------
                                      Sheet A |  493.16 |  487.67 |  -1.11%
                                      Sheet B |  160.91 |  159.67 |  -0.77%
                                      Sheet T |  138.91 |  138.91 |   0.00%
                                Column ranges |  523.27 |  513.76 |  -1.82%
                                Sorted lookup | 16043.2 | 15169.4 |  -5.45%
Sheet A:  change value, add/remove row/column |   18.33 |   15.86 | -13.48%
 Sheet B: change value, add/remove row/column |  157.52 |  147.88 |  -6.12%
                   Column ranges - add column |  165.19 |  160.81 |  -2.65%
                Column ranges - without batch |  501.19 |  500.88 |  -0.06%
                        Column ranges - batch |  124.73 |   126.7 |  +1.58%

Comment thread CHANGELOG.md Outdated
@Tobiadefami
Tobiadefami requested a review from sequba August 6, 2026 22:44
Comment thread src/interpreter/plugin/ArrayPlugin.ts Outdated
Comment thread src/interpreter/plugin/ArrayPlugin.ts Outdated
Comment thread src/interpreter/plugin/ArrayPlugin.ts Outdated
Comment thread src/interpreter/plugin/ArrayPlugin.ts
Comment thread src/Cell.ts Outdated
Comment thread src/interpreter/plugin/ArrayPlugin.ts Outdated
columnsToTake,
rowsToTake,
)
return SimpleRangeValue.onlyRange(resultRange, this.dependencyGraph)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning a range-backed value here changes the result's identity, not just how it is read. SimpleRangeValue.onlyRange leaves _data undefined and sets range, so isAdHoc() becomes false — and coerceRangeToScalar (src/interpreter/ArithmeticHelper.ts:813-832) branches on exactly that flag:

if (arg.isAdHoc()) {
  return arg.data[0]?.[0]
}
const range = arg.range!
if (state.formulaAddress.sheet === range.sheet) {
  if (range.width() === 1) {
    const offset = state.formulaAddress.row - range.start.row

So a TAKE result in scalar context no longer yields its first element; it goes through Excel-style implicit intersection against the source address. Every other array function returns onlyValues and keeps the first-element behaviour.

Verified by running the engine at this commit and at b509e68c7~1, default config (useArrayArithmetic: false), Data!A1:C3 = 1..9:

formula this commit b509e68c7~1
=TAKE(Data!A1:C3,2)+0 #VALUE! "Cell range not allowed." 1
=ABS(TAKE(Data!A1:C3,2)) #VALUE! 1
=TAKE(Data!A1:C3,2)&"" #VALUE! "1"

The controls in the same sheet are unaffected — =SORT(Data!A1:C3)+0, =VSTACK(Data!A1:C3)+0 and =ARRAY_CONSTRAIN(Data!A1:C3,2,3)+0 all return 1 on both revisions. TAKE now behaves like a bare range reference (=Data!A1:C3+0#VALUE!).

Cross-sheet is broken unconditionally, because the state.formulaAddress.sheet === range.sheet guard fails and the function falls through to return undefined:

Sheet1!A1:A2 = =ABS(TAKE(Sheet2!A1:A5,3))
  this commit -> #VALUE!, #VALUE!
  b509e68c7~1 -> 1, 1

When source and formula share a sheet and the source is 1-D, the intersection succeeds — so there is no error, just the wrong number, and which number depends on where the formula sits. With A1:A4 = 10,20,30,40:

B1:B4 = =ROUND(TAKE($A$1:$A$4,2),0)
  this commit -> 10, 20, #VALUE!, #VALUE!
  b509e68c7~1 -> 10, 10, 10, 10

One more consequence: the behaviour now depends on the shape of the input rather than on what TAKE does, because a computed source has range === undefined and falls through to the onlyValues path on line 248. =ROUND(TAKE(VSTACK($A$1:$A$4),2),0) returns 10 in every row — wrapping the source in a no-op changes the answer.

This is the caveat from the earlier thread on this line ("Worth confirming the returned-value path behaves") coming due. The LookupPlugin precedent does not carry over: those onlyRange values are consumed inside doVlookup/doHlookup and never escape, so the flag never reaches a coercion site. TAKE returns one as the formula's value.

Nothing in function-take.spec.ts uses TAKE as an argument to anything, so all 35 cases stay green.

If the laziness is worth keeping, the narrowing needs to happen without changing the result's identity — read only the sub-range's cells and return SimpleRangeValue.onlyValues, or let SimpleRangeValue carry narrowed provenance separately from isAdHoc(). A subRange(topRow, leftCol, height, width) method on SimpleRangeValue would keep that decision behind the class boundary and let ARRAY_CONSTRAIN reuse it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 86a6a93 by materializing only the narrowed range and returning onlyValues; covered in test repo PR 28 by 131f650 and c424c5f.

Comment thread src/interpreter/plugin/ArrayPlugin.ts Outdated
const height = literalRows === undefined ? sourceSize.height : Math.min(sourceSize.height, literalRows)
const width = literalColumns === undefined ? sourceSize.width : Math.min(sourceSize.width, literalColumns)

if (height < 1 || width < 1) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This guard is missing the half that catches non-finite dimensions, so a whole-column or whole-row source silently loses data.

ArraySizePredictor.checkArraySizeForAst returns new ArraySize(range.width(), range.height(), true) for COLUMN_RANGE/ROW_RANGE (src/ArraySize.ts:58-65), and those ranges are constructed with an infinite end — AbsoluteColumnRange sets end.row = Number.POSITIVE_INFINITY (src/AbsoluteCellRange.ts:445), AbsoluteRowRange sets end.col = Number.POSITIVE_INFINITY (:499). When the matching count is not a bare numeric literal, lines 273-274 pass that Infinity straight through, Infinity < 1 is false, and an ArraySize with an infinite axis reaches the vertex.

arrayconstrainArraySize, 134 lines above in this same file, has the clause that catches it:

if (height < 1 || width < 1 || !Number.isInteger(height) || !Number.isInteger(width)) {
  return ArraySize.error()
}

Verified by running the engine at this commit, Data!A1:C3 = 1..9:

formula result expected
=TAKE(Data!1:3,2) [[1],[4]], dims 1x2 [[1,2,3],[4,5,6]]
=TAKE(Data!A:C,,2) [[1,2]], dims 2x1 [[1,2],[4,5],[7,8]]
=ARRAY_CONSTRAIN(Data!1:3,2,3) [[1,2,3],[4,5,6]] correct

No error is raised — B1, C1, B2, C2 are simply null. The values TAKE computes are right and only the reservation is wrong: in the same sheet =COLUMNS(TAKE(Data!1:3,2)) returns 3 and =SUM(TAKE(Data!1:3,2)) returns 21, while the spilled area is one column wide.

The same formulas placed anywhere other than row 1 / column A fail differently, because ArrayFormulaVertex.getRange()AbsoluteCellRange.spanFromOrUndef (src/AbsoluteCellRange.ts:117-126) returns undefined for an infinite span not anchored at index 0, and isThereSpaceForArray then reports no space:

Sheet1!B1 = =TAKE(Data!1:3,2)   ->  #SPILL! "No space for array result."

There is a second, subtler case on the same lines even with a literal count, because the prediction uses the range's declared height while the runtime uses effectiveHeight:

Data = [[1,2,3]]                 // one row
Sheet1!A1 = =TAKE(Data!A:C,3)    // min(Infinity, 3) = 3 predicted, effectiveHeight = 1

The values are correct (1,2,3 in row 1), but A2:C3 are reserved as blanks and isCellPartOfArray(A3) is true; writing anything there turns the formula into #SPILL!. Excel returns a 1x3 result. Clamping the predicted size to the effective dimensions — or reusing ARRAY_CONSTRAIN's Number.isInteger guard so an unbounded source degrades to ArraySize.error() the way it already does for that function — covers both.

No test in function-take.spec.ts uses a whole-column or whole-row source, which is why this is green today.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this in takeArraySize(). Bounded cases like TAKE(A:A, 2) still work, while results that remain unbounded return #VALUE!.

I also added regression tests and documented the Excel difference.

Comment thread src/interpreter/plugin/ArrayPlugin.ts Outdated
new InterpreterState(state.formulaAddress, state.arraysFlag || (metadata?.enableArrayArithmeticForArguments ?? false)),
)

const literalRows = ArrayPlugin.parseTakeLiteralDimension(ast.args[1])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseTakeLiteralDimension matches only AstNodeType.NUMBER and a unary +/- wrapping a NUMBER, so every other way of writing a constant count returns undefined and lines 273-274 fall back to the full source size. The reservation is then decided by the spelling of the count rather than by its value.

Verified at this commit, Data 3x3, Sheet1 = [['=TAKE(Data!A1:C3,<count>)'], [null], ['neighbour']] — every one of these results is a single row that fits in A1:C1:

<count> result
1 1, 2, 3 — correct
"1" #SPILL!
TRUE() #SPILL!
(1) #SPILL!
0+1 #SPILL!
2-1 #SPILL!
200% #SPILL!
--1 #SPILL!
Counts!A1 (= 1) #SPILL!

Even with nothing in the way the surplus is reserved: isCellPartOfArray(A3) is true, and a later setCellContents(A3, 'hello') flips the formula to #SPILL!.

Two of these are inputs the spec explicitly supports — 'coerces numeric text to a count' (function-take.spec.ts:293) and 'coerces TRUE to one' (:300) — and both pass only because they run on an otherwise empty sheet. Note ArraySizePredictor.checkArraySizeForAst unwraps PARENTHESIS (src/ArraySize.ts:115) and runFunctionWithReferenceArgument unwraps it in a while loop, so skipping it here is not a house convention.

The cell-reference case is genuinely unfixable at parse time and an upper bound is the only option there — same situation FILTER and UNIQUE are in. But the constant forms above are all statically known, and the helper this one was modelled on already handles most of them. SequencePlugin.parseLiteralDimension (src/interpreter/plugin/SequencePlugin.ts:41-61) covers NUMBER, STRING, both unary ops, and zero-arg TRUE()/FALSE():

if (node.type === AstNodeType.STRING) {
  const parsed = Number(node.value)
  return Number.isFinite(parsed) ? Math.trunc(parsed) : undefined
}

So =SEQUENCE("2") predicts its size and =TAKE(A1:C3,"2") does not — an inconsistency between two functions in the same category with no reason behind it.

This is now the fourth hand-rolled copy of "extract a static number from an AST node": here, SequencePlugin.parseLiteralDimension, arrayconstrainArraySize (line 136 of this file, NUMBER only), and FormulaParser.handleOffsetHeuristic (src/parser/FormulaParser.ts:766-790, inlined four times). They disagree on capability, and the next one lands with DROP/CHOOSEROWS/EXPAND. Extracting a single signedNumberLiteralValue(ast): number | undefined into src/parser/Ast.ts — which already owns AstNodeType and the unary-op builders — and calling it from all four would fix this class in one place; the Math.abs stays at TAKE's call site since only TAKE wants the magnitude.

Worth pinning whatever the final boundary is with tests, since none of the rows in the table above is covered today.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed locally in cb5bc3c and 3fd5a70. Static text, booleans, parentheses, unary forms, arithmetic, percentages, and literal errors are sized before spill allocation; covered in test repo PR 28.

Comment thread src/Cell.ts Outdated
*/
export enum ErrorType {
/** Calculation error. */
CALC = 'CALC',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to take CALC out of this PR. Adding a member to ErrorType is a breaking change to a public API, and we don't want to cut a major release now.

Why it's breaking

TranslationPackage's constructor validates error and UI keys exhaustively against the enum:

// src/i18n/TranslationPackage.ts
private checkErrors(): void {
  for (const key of Object.values(ErrorType)) {
    if (!(key in this.errors) && (key !== ErrorType.LIC)) {
      throw new MissingTranslationError(`errors.${key}`)
    }
  }
}

So any third-party language pack written against 3.3.0 now fails at registration, before a single formula is evaluated:

HyperFormula.registerLanguage('myPack', packBuiltAgainst_3_3_0)
// MissingTranslationError: Translation for errors.CALC is missing in the translation package you're using.

RawTranslationPackage is public API and custom packs are a documented feature — docs/guide/localizing-functions.md even walks users through building one, and the two example packs on that page list the error keys explicitly, so they throw as of this branch.

Worth being precise about the asymmetry, because it is not obvious: adding a function is not breaking in the same way. checkFunctionTranslations doesn't iterate the registry at all — it only rejects packs that override protected names — so a pack that has never heard of TAKE registers fine and =TAKE(...) simply resolves to #NAME? in that language (FunctionRegistry.getFunction gates on isFunctionTranslated). Verified against this branch, each pack derived from enGB with one key removed:

pack registerLanguage
missing functions.TAKE OK — =TAKE(...)#NAME?
missing functions.SORT OK — =SORT(...)#NAME?
functions: {} (empty) OK
missing errors.CALC throws
missing ui.NEW_SHEET_PREFIX throws

So the TAKE half of this PR is genuinely additive; the CALC half breaks packs on its own, even for users who never call the function. For precedent: #SPILL!, the last translatable error type we added, shipped in 1.0.0.

To be clear — #CALC! is the right answer

Microsoft is explicit: "Excel returns a #CALC! error to indicate an empty array when either rows or columns is 0." This isn't a case where we should pick something else on the merits. It's purely a release-timing constraint.

Interim solution

Use ErrorType.NA and keep the new message:

return new CellError(ErrorType.NA, ErrorMessage.ZeroRowOrColumnCount)

#N/A is the closest fit among the existing types because it is already what this codebase returns for an empty array result — FILTER does exactly that 37 lines above in this same file:

return new CellError(ErrorType.NA, ErrorMessage.EmptyRange)

and UNIQUE and SORT agree. Using #N/A keeps TAKE consistent with its own family instead of introducing a third convention (SEQUENCE(0) returns #VALUE!, which is the other candidate but the odd one out). Keeping ErrorMessage.ZeroRowOrColumnCount preserves the accuracy fix from the earlier round — the message stays correct about what was zero, only the error type changes.

That leaves us with one migration later rather than two, since we'll likely want to move FILTER/UNIQUE/SORT/SEQUENCE to #CALC! at the same time.

What to revert here

  • CALC = 'CALC' in src/Cell.ts
  • the CALC entry in all 17 language packs (this also makes the "every pack ships the untranslated English #CALC!" problem moot for now — when we do add it, the packs need real localized names; Polish Excel uses #OBL!)
  • the #CALC! row in docs/guide/types-of-errors.md
  • the CALC half of the CHANGELOG entry

Keep ErrorMessage.ZeroRowOrColumnCount, and record the deviation in docs/guide/list-of-differences.md — there's already a row of exactly this shape for =SEQUENCE(0) (VALUE / N/A / CALC).

Tracked for the next major in HF-350 (tagged breaking change), which has the full scope and the reasoning above. We'll do it there.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c8924ae. Removed ErrorType.CALC, its entries from all 17 language packs, and the related error documentation and changelog text. TAKE now returns ErrorType.NA with ErrorMessage.ZeroRowOrColumnCount for zero counts.

Added regression coverage in a9f3334 confirming that language packs without a CALC translation register successfully, and documented the Excel deviation.

*
* @param {FunctionArgument | undefined} argument - The argument metadata to inspect.
*/
export function isFunctionArgumentOptional(argument: FunctionArgument | undefined): boolean {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same situation as CALC: the problem is real and the approach is sound, but it's breaking and we can't afford a major right now — so let's solve it locally inside TAKE instead of changing the shared model.

The problem you hit is real

TAKE genuinely needs something the argument-metadata model cannot express: rows must accept a syntactically empty slot (=TAKE(range,,2) → keep all rows) while still rejecting an omitted argument (=TAKE(range) → error, since Excel marks rows Required). emptyAsDefault is inert without a defaultValue, and a defaultValue used to imply optional — so "required, but empty allowed" had no expression.

It's also worth saying why nothing else in the codebase hit this: TAKE is the only function where an emptyAsDefault argument is positionally required. Every other user of the flag puts it on a genuinely optional trailing argument, where "omitted" and "empty" should mean the same thing:

function parameters with emptyAsDefault shape
ADDRESS abs_number, use_a1 trailing, optionalArg: true
SEQUENCE columns, start, step trailing, optional
SORT sort_index, sort_order, by_col trailing, optional
UNIQUE by_col, exactly_once trailing, optional
TAKE rows middle, required

So the gap is genuine, and separating defaults from optionality is the right long-term model.

Why we can't ship it now

isFunctionArgumentOptional changes the meaning of an existing combination on a public interface. The old predicate was optionalArg || defaultValue !== undefined; the new one returns optionalArg verbatim when it is set, so {optionalArg: false, defaultValue: X} flips from optional to required.

FunctionArgument is exported from src/index.ts and custom function plugins are a documented feature. Verified by registering a plugin whose 2nd parameter is {argumentType: NUMBER, optionalArg: false, defaultValue: 42}:

=OPTFN(1)   ->  43        on ebaa2b28a
=OPTFN(1)   ->  #N/A "Wrong number of arguments."   on this branch

getFunctionDetails('OPTFN').parameters[1].optional also flips truefalse, so metadata-driven autocomplete starts advertising the wrong arity.

Two things make it worse than a typical semantic tweak. First, no built-in uses that combination except TAKE itself, so the entire blast radius is user code and nothing in-tree would ever catch a regression. Second, docs/guide/custom-functions.md actively prescribes the combination — its MY_FUNCTION example pairs defaultValue: 10 with optionalArg: false, and the page still states the old rules ("Setting a defaultValue for an argument always makes that argument optional"). A plugin copy-pasted from our own published guide changes arity on upgrade.

Interim solution: check the arity inside TAKE

Revert isFunctionArgumentOptional to the previous predicate, drop optionalArg: false from the rows parameter, and enforce the requirement where it belongs — in the function that has the requirement:

public take(ast: ProcedureAst, state: InterpreterState): InterpreterValue {
  // `rows` carries a default so that a syntactically empty slot keeps every row, which also makes
  // the argument metadata treat it as omittable. TAKE requires it, so the arity is checked here.
  if (ast.args.length < 2) {
    return new CellError(ErrorType.NA, ErrorMessage.WrongArgNumber)
  }

  return this.runFunction(ast.args, state, this.metadata('TAKE'), ...)
}

runFunction still rejects the 4-argument call on its own, and takeArraySize already guards ast.args.length < 2 || > 3, so this is the only gap to close.

I built it and ran it. Every user-visible TAKE behaviour is identical to this branch:

formula this branch with the local check
=TAKE(A1:C3) #N/A #N/A
=TAKE(A1:C3,,2) [[1,2],[4,5],[7,8]] same
=TAKE(A1:C3,2) [[1,2,3],[4,5,6]] same
=TAKE(A1:C3,) / =TAKE(A1:C3,,) full source same
=TAKE(A1:C3,2,) [[1,2,3],[4,5,6]] same
=TAKE(A1:C3,-2,-2) [[5,6],[8,9]] same
=TAKE(A1:C3,0,2) zero-count error same
=TAKE(A1:C3,1,1,1) #N/A same
=OPTFN(1) (third-party plugin above) #N/A 43 — restored

All 35 cases in function-take.spec.ts pass unchanged.

The cost, stated honestly

getFunctionDetails('TAKE').parameters[1].optional becomes true instead of false. The public metadata will advertise rows as optional even though the function rejects the one-argument call — Excel documents it as Required. That's a real (if cosmetic) regression, and it's the price of not touching the shared predicate. The catalogue can't override it, since optionality is derived entirely from implementedFunctions.

Concretely that means three assertions in the tests repo need updating — the full run was 6176 passed / 3 failed, and all three are assertions that encode the new shared-model semantics rather than TAKE's behaviour:

  • optional-parameters.spec.ts"uses a required argument default only for syntactically empty input" (the REQUIREDDEFAULTTEST plugin)
  • function-metadata-api.spec.ts"returns full details for TAKE" (the optional flags)
  • function-metadata-api.spec.ts"derives optionality from the implementation on the fallback path"

Reverting also means docs/guide/custom-functions.md needs no change — it becomes correct again as written.

For the record: the right fix, later

The underlying issue is that defaultValue does two jobs — "value when the argument is omitted" and "value when the slot is empty". Splitting them (a separate emptyValue, or letting emptyAsDefault carry its own value) lets TAKE declare no defaultValue at all: it stays required under the existing optionality rule, empty slots still work, no existing flag changes meaning, and nothing third-party breaks. I prototyped that too and it gives optional: [false, false, true] with the full suite green apart from the same three assertions. Worth a backlog item alongside the #CALC! one — happy to file it if you want.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restored the established behavior where arguments with a defaultValue are treated as optional, and removed optionalArg: false from TAKE’s rows metadata

The required argument-position rule is now localized to TAKE: TAKE(array) returns #N/A with WrongArgNumber, while TAKE(array, , columns) remains valid and uses the default row count.

Added coverage for default-value optionality, public function metadata, and TAKE’s argument-count behavior.
Implementation: 6804d04
Tests: 9685c61

Comment thread src/i18n/languages/daDK.ts Outdated
Comment thread src/i18n/languages/ptPT.ts
Tobiadefami and others added 13 commits August 13, 2026 11:28
TAKE shipped with the English name in 10 of 16 packs while 6 carried a
translation, and VSTACK/HSTACK were English in all 16. Microsoft localizes
all three in most locales, so a user could not type the name their Excel
uses.

Names taken from Microsoft's localized "Excel functions (alphabetical)"
page, one locale at a time. Each row there links to the function's own page
using the English slug in the href while the link text is the localized
name, so the lookup is exact:

    <a href="functions/take-function">WYCINEK</a>

Left as English where Microsoft itself does not translate: TAKE, VSTACK and
HSTACK in Indonesian, and VSTACK/HSTACK in Swedish.

Note that a function's own localized page is not a usable source: for
several locales its syntax block still shows the English name even though
the prose and argument names are translated (the French page shows
"=TAKE(tableau, lignes,[colonnes])" while the product uses PRENDRE).

DEV_DOCS records the lookup method and adds the governing policy: ship a
localized name only when it can be confirmed against the product, and keep
the English name otherwise, since an invented name matches nothing, reads
plausibly enough to be typed first, and fails as #NAME?.

No changelog entry: TAKE, VSTACK and HSTACK are all still in [Unreleased],
so no wrong name has been released.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Tobiadefami
Tobiadefami force-pushed the feature/take-function branch from 1118288 to a18429c Compare August 13, 2026 10:47
Comment thread src/interpreter/plugin/ArrayPlugin.ts Outdated
* @param {Ast | undefined} argument - The count argument to inspect before evaluation.
* @returns {TakeLiteralDimension} The literal value, an invalid-literal marker, or an unresolved marker.
*/
private parseTakeLiteralDimension(argument: Ast | undefined): TakeLiteralDimension {

@marcin-kordas-hoc marcin-kordas-hoc Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseTakeLiteralDimension only recognizes a bare NUMBER/STRING and a single unary +/-, so every other way of writing a constant count returns unresolved and falls back to reserving the full source size instead of the literal's value. TAKE(range,TRUE()), TAKE(range,(1)) and TAKE(range,0+1) therefore produce a false #SPILL! when there is content near the true (smaller) result — reproduced against this PR's HEAD.

Real Excel evaluates all three of those as 1 (measured against Excel through the Graph API), so this is a genuine behavioural deviation rather than an HF convention. Worth noting the boundary so you don't over-fix: 200% is not part of this — real Excel rejects that one too.

Could you extend the parser to cover these forms, plus regression tests for each spelling?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cb5bc3c and 3fd5a70; covered in test repo PR 28 by cf89480, 554ece1, and c781315.

* @param ast
* @param state
*/
public takeArraySize(ast: ProcedureAst, state: InterpreterState): ArraySize {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TAKE with a whole-column source (e.g. TAKE(A:A,,1)) returns #VALUE! "Cell range not allowed" here, even when the source is on the same sheet as the formula — confirmed live via MS Graph that real Excel spills correctly in exactly that case.

This does not look like an engine-wide limitation: SORT, UNIQUE, and FILTER in this same codebase already handle whole-column sources correctly, same-sheet and cross-sheet alike (checked all three live on current develop). Would you be open to reusing whatever SortPlugin/UniquePlugin do differently in their array-size prediction, so takeArraySize does not need a blanket rejection of non-finite dimensions? Happy to point at the exact lines if useful.

Separately: the cross-sheet case (Data!A:A, the example already in this PR's own list-of-differences.md) does genuinely fail in real Excel too (#SPILL!) — so that doc row's "Spills the whole column from row 1" claim is inaccurate for its own example. Worth correcting once the same-sheet case above is fixed.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cb5bc3c; covered in test repo PR 28 by cf89480 and 8047c14. Direct Excel Online verification showed cross-sheet sources also spill from the sheet edge, so the docs reflect edge-based placement.

@marcin-kordas-hoc

Copy link
Copy Markdown
Collaborator

Nice catch fixing VSTACK/HSTACK to use Excel's real localized names across 14 languages — spot-checked two directly against Microsoft's own docs (French 'ASSEMB.V', German 'VSTAPELN') and both are correct.

Only ask: could you mention this in the PR description? Right now it only shows up in the auto-generated bot summary, and it's a real (good) breaking change for anyone who was using the placeholder English names in those locales before.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4459034. Configure here.

Comment thread src/interpreter/plugin/ArrayPlugin.ts Outdated
@marcin-kordas-hoc

Copy link
Copy Markdown
Collaborator

The description still describes the CALC design that was dropped: "It also introduces the CALC error type and its #CALC! representation in all built-in language packs. For now, #CALC! is produced only by TAKE when a row or column count evaluates to zero", plus the test-plan bullet "Zero row and column counts returning #CALC!". The code returns ErrorType.NA with ZeroRowOrColumnCount — the right call per the thread above; the description just didn't follow. Worth fixing, since it's the first thing a reviewer reads and it points at behaviour that is deliberately absent.

Two small leftovers from that same removal:

  • docs/guide/types-of-errors.md is still in the diff, but the only remaining change in it is a trailing newline at EOF. Either drop it from the PR or keep it on purpose.
  • Please don't "fix" the list-of-differences.md row for TAKE while you're in there — HF N/A vs Excel CALC is correct as written (I checked it against real Excel).

@Tobiadefami

Tobiadefami commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

@marcin-kordas-hoc Thanks for flagging this. I moved the VSTACK/HSTACK localization fix into its own PR so the breaking language-pack migration can be reviewed separately: #1748 (companion tests: 44). The changes are no longer part of this TAKE PR.

@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.35%. Comparing base (61ead73) to head (3fd5a70).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1722      +/-   ##
===========================================
+ Coverage    97.31%   97.35%   +0.04%     
===========================================
  Files          195      195              
  Lines        15719    15847     +128     
  Branches      3455     3443      -12     
===========================================
+ Hits         15297    15428     +131     
- Misses         414      419       +5     
+ Partials         8        0       -8     
Files with missing lines Coverage Δ
src/error-message.ts 100.00% <100.00%> (ø)
src/i18n/languages/csCZ.ts 100.00% <ø> (ø)
src/i18n/languages/daDK.ts 100.00% <ø> (ø)
src/i18n/languages/deDE.ts 100.00% <ø> (ø)
src/i18n/languages/enGB.ts 100.00% <ø> (ø)
src/i18n/languages/esES.ts 100.00% <ø> (ø)
src/i18n/languages/fiFI.ts 100.00% <ø> (ø)
src/i18n/languages/frFR.ts 100.00% <ø> (ø)
src/i18n/languages/huHU.ts 100.00% <ø> (ø)
src/i18n/languages/idID.ts 100.00% <ø> (ø)
... and 10 more

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants