Fix TEXTJOIN PR: remove tests from public repo, fix docs ordering#1637
Closed
marcin-kordas-hoc wants to merge 0 commit intomasterfrom
Closed
Fix TEXTJOIN PR: remove tests from public repo, fix docs ordering#1637marcin-kordas-hoc wants to merge 0 commit intomasterfrom
marcin-kordas-hoc wants to merge 0 commit intomasterfrom
Conversation
7d1f307 to
6de904b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Addresses review feedback from @sequba on PR #1625 (Add TEXTJOIN function).
sequba pointed out the test placement issue twice:
This branch contains the full TEXTJOIN implementation from
feature/TEXTJOIN_w_docswith the following corrections applied.Changes
test/textjoin.spec.ts— tests belong in the privatehyperformula-testsrepositorytest/smoke.spec.ts— restored smoke tests to their original minimal statedocs/guide/built-in-functions.md— moved TEXTJOIN entry from between SUBSTITUTE/T to the correct position between TEXT/TRIMtest/fetch-tests.shandtest:setup-privatenpm script (synced from develop) for cloning the private test repo/test/hyperformula-tests/to.gitignoretest/hyperformula-tests/textjoin.spec.ts(29 tests, all passing) — needs to be pushed to thehyperformula-testsprivate repoImplementation review
The TEXTJOIN implementation was reviewed against other function patterns in the codebase:
implementedFunctions) usesrepeatLastArgs: 1withFunctionArgumentType.ANYfor delimiter and text args — correct pattern for variable-arg functions that handle ranges manuallyflattenArgToStringshelper follows the same range iteration pattern as SUMPRODUCT, DateTimePlugin, and NumericAggregationPlugin (arg instanceof SimpleRangeValue ? arg.valuesFromTopLeftCorner() : [arg])coerceScalarToString(EmptyValue →'') with subsequent filtering is correctHow did you test your changes?
npm run compile— TypeScript compiles without errorsnpm run lint— 0 errors (only pre-existing warnings)npx jest test/smoke.spec.ts— all 4 smoke tests passnpx jest test/hyperformula-tests/textjoin.spec.ts— all 29 TEXTJOIN tests pass (locally)Types of changes
Related issues:
Checklist: