Skip to content

Add CONTRIBUTING file - #69

Merged
scarletfog merged 6 commits into
masterfrom
feature/issue-52
Dec 12, 2019
Merged

scarletfog merged 6 commits into
masterfrom
feature/issue-52

Conversation

@scarletfog

@scarletfog scarletfog commented Dec 8, 2019 •

Copy link
Copy Markdown
Contributor

Description

Formatted view: https://github.com/handsontable/hyperformula/blob/feature/issue-52/CONTRIBUTING.md
Contributing file was added, it should be adapted though; few questions:

  • Is point 3 mandatory (or should be)?
  • How should we test? I found specs for tests but no e2e ones (?).
  • I recall that tslint would be deprecated https://medium.com/palantir/tslint-in-2019-1a144c2317a9 - a switch to typescript-eslint. Is point 5 relevant then?
  • Old link to git-flow (from hot project) does not seem to work, I replaced it with another one but I am not 100% sure it is correct.

@wojciechczerniak

Copy link
Copy Markdown
Contributor
  • Is point 3 mandatory (or should be)?

It should. We always work on develop branch. master should be clear of bugs and ready to release. I know that HyperFormula is not following this at the moment. We will have to switch our workflow in January.

  • How should we test? I found specs for tests but no e2e ones (?).

I think that they are mixed. Here you have an example where the engine is created, sheet filled with data and public API used to extract the value, IMO far from a unit test

it('accepts exactly one argument', () => {
const engine = HyperFormula.buildFromArray([['=COLUMNS()', '=COLUMNS(A1:B1, A2:B2)']])
expect(engine.getCellValue(adr('A1'))).toEqual(new CellError(ErrorType.NA))
expect(engine.getCellValue(adr('B1'))).toEqual(new CellError(ErrorType.NA))
})

Unless we switch to eslint this is the correct flow for now. Then we need to remember that the task for switching the tool should change the contributing guide as well.

The best solution is to add npm run lint command and keep the guide implementation agnostic.

  • Old link to git-flow (from hot project) does not seem to work, I replaced it with another one but I am not 100% sure it is correct.

😱 Shouldn't we fix it in Handsontable repo then? I think that @jansiegel can verify the article, but I don't know if he reads the notifications 🙄 Ping! 🛎

@scarletfog

Copy link
Copy Markdown
Contributor Author

@scarletfog scarletfog self-assigned this Dec 9, 2019
@scarletfog

Copy link
Copy Markdown
Contributor Author

@wojciechczerniak
point 3 ✔️
About the tests - I left it as a non-specific - "plz add tests", not sure if it does the job for now?
We got Makefile + yarn 🐈, a dummy question - should I squeeze npm run lint into Makefile?
Link changed as Jan suggested ✔️

@wojciechczerniak

Copy link
Copy Markdown
Contributor

About the tests - I left it as a non-specific - "plz add tests", not sure if it does the job for now?
We got Makefile + yarn 🐈, a dummy question - should I squeeze npm run lint into Makefile?

Oh I see now, it's already added to makefile. No need for npm run lint. And we can remove yarn lint from the guide. make lint is enough and implementation agnostic. If we change linter we will change the command in the makefile.

Comment thread CONTRIBUTING.md Outdated
scarletfog and others added 2 commits December 10, 2019 15:31
Co-Authored-By: Wojciech Czerniak <wojciech.czerniak@gmail.com>
@scarletfog

Copy link
Copy Markdown
Contributor Author

@wojciechczerniak may I get ✅ on this one? Or do you see something still to do?

@wojciechczerniak
wojciechczerniak self-requested a review December 12, 2019 19:13
@scarletfog
scarletfog merged commit 51c4e0b into master Dec 12, 2019
@scarletfog
scarletfog deleted the feature/issue-52 branch December 12, 2019 19:14
sequba pushed a commit that referenced this pull request Apr 3, 2026
## Problem

HyperFormula was missing the SEQUENCE dynamic array function for
generating sequential number arrays.

## Fix

Implements `SEQUENCE(rows, [cols], [start], [step])` as a new
`SequencePlugin`:

- Returns a rows×cols array of sequential numbers, filled row-major
- Parse-time array size prediction via `sequenceArraySize()` — handles
NUMBER and STRING literals; non-literal args (cell refs, formulas)
return `#VALUE!` (architectural limitation: array size must be known at
parse time)
- Error types match Excel: negative dims → `#VALUE!`, zero dims →
`#NUM!` (mapped from Excel's `#CALC!`)
- `emptyAsDefault: true` on optional params — empty args like
`=SEQUENCE(3,,,)` use declared defaults
- i18n for all 17 languages with proper Excel-localized names

## Changed files

| File | Change |
|------|--------|
| `src/interpreter/plugin/SequencePlugin.ts` | New plugin: `sequence()`
+ `sequenceArraySize()` |
| `src/interpreter/plugin/index.ts` | Plugin registration |
| `src/i18n/languages/*.ts` (17 files) | SEQUENCE translations |
| `docs/guide/built-in-functions.md` | SEQUENCE row in Array functions
table |
| `docs/guide/release-notes.md` | Unreleased section |
| `CHANGELOG.md` | Added entry |
| `test/smoke.spec.ts` | 3 smoke tests |
| `test/fetch-tests.sh` | Robustness fix for `git pull` |

## Tests

Regression tests in `handsontable/hyperformula-tests` (branch
`feature/SEQUENCE`):

| Group | Tests | Coverage |
|-------|-------|----------|
| Core sanity | #1–#8 | Basic usage, MS docs examples |
| Default parameters | #9–#13 | Omitted cols/start/step |
| Empty args | #14–#21 | emptyAsDefault behavior |
| Step variants | #22–#28 | Zero, negative, fractional step |
| Truncation | #29–#35 | Fractional dims, trunc-to-zero |
| Error conditions | #36–#48 | Zero/negative dims, text, arity,
propagation |
| Type coercion | #49–#59 | Booleans, strings, cell refs, empty cells |
| Large sequences | #60–#63 | 100×100, 1000×1, 1×1000 |
| Fill order | #64–#69 | Row-major verification |
| Function combos | #70–#74 | SUM, AVERAGE, MAX, MIN, COUNT |
| Behavioral | #75–#80 | Max dims, spill |
| Dynamic args | #81–#82 | Architectural limitation (cell ref → #VALUE!)
|

- 82/82 PASS confirmed in Excel desktop (Microsoft 365)
- 3 smoke tests in `test/smoke.spec.ts`



<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Adds a new array-producing built-in (`SEQUENCE`) with parse-time size
prediction rules; mistakes here can affect array vertex creation and
spill/error behavior across formulas. Remaining changes are
documentation/i18n updates plus a minor test script tweak.
> 
> **Overview**
> Adds the `SEQUENCE(rows, [cols], [start], [step])` built-in via a new
`SequencePlugin`, generating row-major numeric arrays and enforcing
dimension/max-sheet limits with appropriate errors.
> 
> Introduces parse-time result sizing (`sequenceArraySize`) that only
accepts literal `rows`/`cols` (non-literal dimensions now yield
`#VALUE!` due to unknown output size), and wires the plugin into the
interpreter exports.
> 
> Updates changelog and docs to list `SEQUENCE`, adds function name
translations across all language packs, and adjusts
`test/fetch-tests.sh` to pull explicitly from `origin` for the current
branch.
> 
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
b08cd79. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

---------

Co-authored-by: Claude <noreply@anthropic.com>
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.

2 participants