Skip to content

Fix TEXTJOIN PR: remove tests from public repo, fix docs ordering#1637

Closed
marcin-kordas-hoc wants to merge 0 commit intomasterfrom
cursor/funkcja-textjoin-poprawno-399f
Closed

Fix TEXTJOIN PR: remove tests from public repo, fix docs ordering#1637
marcin-kordas-hoc wants to merge 0 commit intomasterfrom
cursor/funkcja-textjoin-poprawno-399f

Conversation

@marcin-kordas-hoc
Copy link
Collaborator

@marcin-kordas-hoc marcin-kordas-hoc commented Mar 16, 2026

Context

Addresses review feedback from @sequba on PR #1625 (Add TEXTJOIN function).

sequba pointed out the test placement issue twice:

  1. March 2 — "Do not add tests to [smoke.spec.ts]. We want to keep it very basic."
  2. March 13 — "Please, move tests to hyperformula-test repository"

This branch contains the full TEXTJOIN implementation from feature/TEXTJOIN_w_docs with the following corrections applied.

Changes

  • Removed test/textjoin.spec.ts — tests belong in the private hyperformula-tests repository
  • Removed TEXTJOIN tests from test/smoke.spec.ts — restored smoke tests to their original minimal state
  • Fixed alphabetical ordering in docs/guide/built-in-functions.md — moved TEXTJOIN entry from between SUBSTITUTE/T to the correct position between TEXT/TRIM
  • Added test/fetch-tests.sh and test:setup-private npm script (synced from develop) for cloning the private test repo
  • Added /test/hyperformula-tests/ to .gitignore
  • Prepared test/hyperformula-tests/textjoin.spec.ts (29 tests, all passing) — needs to be pushed to the hyperformula-tests private repo

Implementation review

The TEXTJOIN implementation was reviewed against other function patterns in the codebase:

  • Function metadata (implementedFunctions) uses repeatLastArgs: 1 with FunctionArgumentType.ANY for delimiter and text args — correct pattern for variable-arg functions that handle ranges manually
  • flattenArgToStrings helper follows the same range iteration pattern as SUMPRODUCT, DateTimePlugin, and NumericAggregationPlugin (arg instanceof SimpleRangeValue ? arg.valuesFromTopLeftCorner() : [arg])
  • Empty value handling via coerceScalarToString (EmptyValue → '') with subsequent filtering is correct
  • Error propagation, delimiter cycling, and 32,767 character limit all work correctly
  • All 17 i18n translations are present and correct

How did you test your changes?

  • npm run compile — TypeScript compiles without errors
  • npm run lint — 0 errors (only pre-existing warnings)
  • npx jest test/smoke.spec.ts — all 4 smoke tests pass
  • npx jest test/hyperformula-tests/textjoin.spec.ts — all 29 TEXTJOIN tests pass (locally)

Types of changes

  • New feature or improvement (a non-breaking change that adds functionality)
  • Additional language file, or a change to an existing language file (translations)
  • Change to the documentation

Related issues:

  1. Fixes Add TEXTJOIN function #1625

Checklist:

  • I have reviewed the guidelines about Contributing to HyperFormula and I confirm that my code follows the code style of this project.
  • My change is compliant with the OpenDocument standard.
  • My change is compatible with Microsoft Excel.
  • My change is compatible with Google Sheets.
  • I described my changes in the CHANGELOG.md file.
  • My changes require a documentation update.
  • My changes require a migration guide.
Open in Web Open in Cursor 

@cursor cursor bot closed this Mar 16, 2026
@cursor cursor bot force-pushed the cursor/funkcja-textjoin-poprawno-399f branch from 7d1f307 to 6de904b Compare March 16, 2026 13:02
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.

1 participant