rows/columns fixed to work with arrays (#677)

* parsing of matrices * size prediction * evaluation, parsing, unparsing * tests * doc * linter * compiles * linter * small refactor * kind of works * cleanup * linter * extra config options * tests * config * rows/columns fixed to work with arrays * doc * Update CHANGELOG.md * docs * compiles
handsontable · May 28, 2021 · 3050d9f · 3050d9f
1 parent b01927e
commit 3050d9f
Show file tree

Hide file tree

Showing 6 changed files with 104 additions and 33 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -31,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 - Fixed an issue with arrays and cruds. (#651)
+- Fixed handling of arrays for ROWS/COLUMNS functions. (#677)
 - Fixed an issue with nested namedexpressions. (#679)
 
 ## [0.6.2] - 2021-05-26

diff --git a/src/interpreter/plugin/FunctionPlugin.ts b/src/interpreter/plugin/FunctionPlugin.ts
@@ -411,7 +411,7 @@ export abstract class FunctionPlugin implements FunctionPluginTypecheck<Function
     args: Ast[],
     state: InterpreterState,
     metadata: FunctionMetadata,
-    noArgCallback: () => InternalScalarValue | RawScalarValue,
+    noArgCallback: () => InternalScalarValue,
     referenceCallback: (reference: SimpleCellAddress) => InternalScalarValue,
     nonReferenceCallback: (...arg: any) => InternalScalarValue = () => new CellError(ErrorType.NA, ErrorMessage.CellRefExpected)
   ) => {

diff --git a/src/interpreter/plugin/InformationPlugin.ts b/src/interpreter/plugin/InformationPlugin.ts
@@ -331,18 +331,32 @@ export class InformationPlugin extends FunctionPlugin implements FunctionPluginT
    * @param ast
    * @param _state
    */
-  public columns(ast: ProcedureAst, _state: InterpreterState): InternalScalarValue {
+  public columns(ast: ProcedureAst, state: InterpreterState): InternalScalarValue {
     if (ast.args.length !== 1) {
       return new CellError(ErrorType.NA, ErrorMessage.WrongArgNumber)
     }
     if (ast.args.some((astIt) => astIt.type === AstNodeType.EMPTY)) {
       return new CellError(ErrorType.NUM, ErrorMessage.EmptyArg)
     }
-    const rangeAst = ast.args[0]
-    if (rangeAst.type === AstNodeType.CELL_RANGE) {
-      return rangeAst.end.col - rangeAst.start.col + 1
+    let argAst = ast.args[0]
+    while(argAst.type === AstNodeType.PARENTHESIS) {
+      argAst = argAst.expression
+    }
+    if (argAst.type === AstNodeType.CELL_RANGE || argAst.type === AstNodeType.COLUMN_RANGE) {
+      return argAst.end.col - argAst.start.col + 1
+    } else if(argAst.type === AstNodeType.CELL_REFERENCE) {
+      return 1
+    } else if(argAst.type === AstNodeType.ROW_RANGE) {
+      return this.config.maxColumns
     } else {
-      return new CellError(ErrorType.VALUE, ErrorMessage.CellRangeExpected)
+      const val = this.evaluateAst(argAst, state)
+      if(val instanceof SimpleRangeValue) {
+        return val.width()
+      } else if (val instanceof CellError){
+        return val
+      } else {
+        return 1
+      }
     }
   }
 
@@ -369,25 +383,39 @@ export class InformationPlugin extends FunctionPlugin implements FunctionPluginT
    * @param ast
    * @param _state
    */
-  public rows(ast: ProcedureAst, _state: InterpreterState): InternalScalarValue {
+  public rows(ast: ProcedureAst, state: InterpreterState): InternalScalarValue {
     if (ast.args.length !== 1) {
       return new CellError(ErrorType.NA, ErrorMessage.WrongArgNumber)
     }
     if (ast.args.some((astIt) => astIt.type === AstNodeType.EMPTY)) {
       return new CellError(ErrorType.NUM, ErrorMessage.EmptyArg)
     }
-    const rangeAst = ast.args[0]
-    if (rangeAst.type === AstNodeType.CELL_RANGE) {
-      return rangeAst.end.row - rangeAst.start.row + 1
+    let argAst = ast.args[0]
+    while(argAst.type === AstNodeType.PARENTHESIS) {
+      argAst = argAst.expression
+    }
+    if (argAst.type === AstNodeType.CELL_RANGE || argAst.type === AstNodeType.ROW_RANGE) {
+      return argAst.end.row - argAst.start.row + 1
+    } else if(argAst.type === AstNodeType.CELL_REFERENCE) {
+      return 1
+    } else if(argAst.type === AstNodeType.COLUMN_RANGE) {
+      return this.config.maxRows
     } else {
-      return new CellError(ErrorType.VALUE, ErrorMessage.CellRangeExpected)
+      const val = this.evaluateAst(argAst, state)
+      if(val instanceof SimpleRangeValue) {
+        return val.height()
+      } else if (val instanceof CellError){
+        return val
+      } else {
+        return 1
+      }
     }
   }
 
   /**
    * Corresponds to INDEX(range;)
    *
-   * Returns number of rows in provided range of cells
+   * Returns specific position in 2d array.
    *
    * @param ast
    * @param state

diff --git a/test/interpreter/function-columns.spec.ts b/test/interpreter/function-columns.spec.ts
@@ -1,5 +1,4 @@
-import {HyperFormula} from '../../src'
-import {ErrorType} from '../../src/Cell'
+import {ErrorType, HyperFormula} from '../../src'
 import {ErrorMessage} from '../../src/error-message'
 import {adr, detailedError} from '../testUtils'
 
@@ -17,34 +16,55 @@ describe('Function COLUMNS', () => {
     expect(engine.getCellValue(adr('A1'))).toEqual(3)
   })
 
-  // Inconsistency with Product 1
-  it('doesnt work with scalars', () => {
+  it('works for column range', () => {
+    const engine = HyperFormula.buildFromArray([['=COLUMNS(A:C)']])
+
+    expect(engine.getCellValue(adr('A1'))).toEqual(3)
+  })
+
+  it('works for row range', () => {
+    const engine = HyperFormula.buildFromArray([['=COLUMNS(1:2)']])
+
+    expect(engine.getCellValue(adr('A1'))).toEqual(engine.getConfig().maxColumns)
+  })
+
+  it('works for array', () => {
+    const engine = HyperFormula.buildFromArray([['=COLUMNS({1,2,3})']])
+
+    expect(engine.getCellValue(adr('A1'))).toEqual(3)
+  })
+
+  it('works with cell reference', () => {
     const engine = HyperFormula.buildFromArray([['=COLUMNS(A1)']])
 
-    expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.VALUE, ErrorMessage.CellRangeExpected))
+    expect(engine.getCellValue(adr('A1'))).toEqual(1)
+  })
+
+  it('error when nested cycle', () => {
+    const engine = HyperFormula.buildFromArray([['=COLUMNS(A1+1)']])
+
+    expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.CYCLE))
   })
 
-  // Inconsistency with Product 1
-  it('propagate only direct errors', () => {
+  it('propagates only direct errors', () => {
     const engine = HyperFormula.buildFromArray([
       ['=4/0'],
       ['=COLUMNS(4/0)'],
       ['=COLUMNS(A1)'],
     ])
 
-    expect(engine.getCellValue(adr('A2'))).toEqualError(detailedError(ErrorType.VALUE, ErrorMessage.CellRangeExpected))
-    expect(engine.getCellValue(adr('A3'))).toEqualError(detailedError(ErrorType.VALUE, ErrorMessage.CellRangeExpected))
+    expect(engine.getCellValue(adr('A2'))).toEqualError(detailedError(ErrorType.DIV_BY_ZERO))
+    expect(engine.getCellValue(adr('A3'))).toEqual(1)
   })
 
-  // Inconsistency with Product 1
-  it('doesnt work with formulas', () => {
+  it('works with formulas', () => {
     const engine = HyperFormula.buildFromArray([
       ['1', '1'],
       ['1', '1'],
       ['=COLUMNS(MMULT(A1:B2, A1:B2))'],
     ])
 
-    expect(engine.getCellValue(adr('A3'))).toEqualError(detailedError(ErrorType.VALUE, ErrorMessage.CellRangeExpected))
+    expect(engine.getCellValue(adr('A3'))).toEqual(2)
   })
 
   it('should work when adding column', () => {

diff --git a/test/interpreter/function-rows.spec.ts b/test/interpreter/function-rows.spec.ts
@@ -17,34 +17,50 @@ describe('Function ROWS', () => {
     expect(engine.getCellValue(adr('A1'))).toEqual(2)
   })
 
-  // Inconsistency with Product 1
-  it('doesnt work with scalars', () => {
+  it('works for row range', () => {
+    const engine = HyperFormula.buildFromArray([['=ROWS(1:3)']])
+
+    expect(engine.getCellValue(adr('A1'))).toEqual(3)
+  })
+
+  it('works for column range', () => {
+    const engine = HyperFormula.buildFromArray([['=ROWS(A:C)']])
+
+    expect(engine.getCellValue(adr('A1'))).toEqual(engine.getConfig().maxRows)
+  })
+
+  it('works for array', () => {
+    const engine = HyperFormula.buildFromArray([['=ROWS({1;2;3})']])
+
+    expect(engine.getCellValue(adr('A1'))).toEqual(3)
+  })
+
+  it('works with cell reference', () => {
     const engine = HyperFormula.buildFromArray([['=ROWS(A1)']])
 
-    expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.VALUE, ErrorMessage.CellRangeExpected))
+    expect(engine.getCellValue(adr('A1'))).toEqual(1)
   })
 
-  // Inconsistency with Product 1
-  it('propagate only direct errors', () => {
+  it('propagates only direct errors', () => {
     const engine = HyperFormula.buildFromArray([
       ['=4/0'],
       ['=ROWS(4/0)'],
       ['=ROWS(A1)'],
     ])
 
-    expect(engine.getCellValue(adr('A2'))).toEqualError(detailedError(ErrorType.VALUE, ErrorMessage.CellRangeExpected))
-    expect(engine.getCellValue(adr('A3'))).toEqualError(detailedError(ErrorType.VALUE, ErrorMessage.CellRangeExpected))
+    expect(engine.getCellValue(adr('A2'))).toEqualError(detailedError(ErrorType.DIV_BY_ZERO))
+    expect(engine.getCellValue(adr('A3'))).toEqual(1)
   })
 
   // Inconsistency with Product 1
-  it('doesnt work with formulas', () => {
+  it('works with formulas', () => {
     const engine = HyperFormula.buildFromArray([
       ['1', '1'],
       ['1', '1'],
       ['=ROWS(MMULT(A1:B2, A1:B2))'],
     ])
 
-    expect(engine.getCellValue(adr('A3'))).toEqualError(detailedError(ErrorType.VALUE, ErrorMessage.CellRangeExpected))
+    expect(engine.getCellValue(adr('A3'))).toEqual(2)
   })
 
   it('should work when adding column', () => {

diff --git a/test/interpreter/function-sheet.spec.ts b/test/interpreter/function-sheet.spec.ts
@@ -79,4 +79,10 @@ describe('Function SHEET', () => {
 
     expect(engine.getCellValue(adr('A1'))).toEqual(1)
   })
+
+  it('should make cycle for non-refs', () => {
+    const engine = HyperFormula.buildFromArray([['=SHEET(1+A1)']])
+
+    expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.CYCLE))
+  })
 })