Refactor

Author: sequba

src/interpreter/ArithmeticHelper.ts

@@ -47,7 +47,7 @@ export class ArithmeticHelper {
   constructor(
     private readonly config: Config,
     private readonly dateTimeHelper: DateTimeHelper,
-    public readonly numberLiteralsHelper: NumberLiteralHelper,
+    private readonly numberLiteralsHelper: NumberLiteralHelper,
   ) {
     this.collator = collatorFromConfig(config)
     this.actualEps = config.smartRounding ? config.precisionEpsilon : 0
@@ -222,6 +222,41 @@ export class ArithmeticHelper {
     )
   }
 
+  /**
+   * Parses a string to a number, supporting percentages, currencies, numeric strings, and date/time formats.
+   * Unlike coerceNonDateScalarToMaybeNumber, this also handles scientific notation with uppercase E.
+   */
+  public parseStringToNumber(input: string): Maybe<ExtendedNumber> {
+    const trimmedInput = input.trim()
+
+    // Try percentage
+    const percentResult = this.coerceStringToMaybePercentNumber(trimmedInput)
+    if (percentResult !== undefined) {
+      return percentResult
+    }
+
+    // Try currency
+    const currencyResult = this.coerceStringToMaybeCurrencyNumber(trimmedInput)
+    if (currencyResult !== undefined) {
+      return currencyResult
+    }
+
+    // Try plain number (normalize scientific notation E to e)
+    const normalizedInput = trimmedInput.replace(/E/g, 'e')
+    const numberResult = this.numberLiteralsHelper.numericStringToMaybeNumber(normalizedInput)
+    if (numberResult !== undefined) {
+      return numberResult
+    }
+
+    // Try date/time
+    const dateTimeResult = this.dateTimeHelper.dateStringToDateNumber(trimmedInput)
+    if (dateTimeResult !== undefined) {
+      return dateTimeResult
+    }
+
+    return undefined
+  }
+
   public coerceNonDateScalarToMaybeNumber(arg: InternalScalarValue): Maybe<ExtendedNumber> {
     if (arg === EmptyValue) {
       return 0
@@ -250,7 +285,7 @@ export class ArithmeticHelper {
     }
   }
 
-  public coerceStringToMaybePercentNumber(input: string): Maybe<PercentNumber> {
+  private coerceStringToMaybePercentNumber(input: string): Maybe<PercentNumber> {
     const trimmedInput = input.trim()
 
     if (trimmedInput.endsWith('%')) {
@@ -264,7 +299,7 @@ export class ArithmeticHelper {
     return undefined
   }
 
-  public coerceStringToMaybeCurrencyNumber(input: string): Maybe<CurrencyNumber> {
+  private coerceStringToMaybeCurrencyNumber(input: string): Maybe<CurrencyNumber> {
     const matchedCurrency = this.currencyMatcher(input.trim())
 
     if (matchedCurrency !== undefined) {

src/interpreter/plugin/TextPlugin.ts

@@ -414,14 +414,14 @@ export class TextPlugin extends FunctionPlugin implements FunctionPluginTypechec
         // Try parsing parentheses notation for negative numbers: "(123)" -> -123
         const parenthesesMatch = /^\(([^()]+)\)$/.exec(trimmedArg)
         if (parenthesesMatch) {
-          const innerValue = this.parseStringToNumber(parenthesesMatch[1])
+          const innerValue = this.arithmeticHelper.parseStringToNumber(parenthesesMatch[1])
           if (innerValue !== undefined) {
             return -innerValue
           }
         }
 
         // Try standard parsing
-        const parsedValue = this.parseStringToNumber(trimmedArg)
+        const parsedValue = this.arithmeticHelper.parseStringToNumber(trimmedArg)
         if (parsedValue !== undefined) {
           return parsedValue
         }
@@ -433,37 +433,6 @@ export class TextPlugin extends FunctionPlugin implements FunctionPluginTypechec
     })
   }
 
-  /**
-   * Parses a string to a number, supporting percentages, currencies, numeric strings, and date/time formats.
-   */
-  private parseStringToNumber(input: string): ExtendedNumber | undefined {
-    // Try percentage
-    const percentResult = this.arithmeticHelper.coerceStringToMaybePercentNumber(input)
-    if (percentResult !== undefined) {
-      return percentResult
-    }
-
-    // Try currency
-    const currencyResult = this.arithmeticHelper.coerceStringToMaybeCurrencyNumber(input)
-    if (currencyResult !== undefined) {
-      return currencyResult
-    }
-
-    // Try plain number
-    const numberResult = this.arithmeticHelper.numberLiteralsHelper.numericStringToMaybeNumber(input.trim())
-    if (numberResult !== undefined) {
-      return numberResult
-    }
-
-    // Try date/time
-    const dateTimeResult = this.dateTimeHelper.dateStringToDateNumber(input)
-    if (dateTimeResult !== undefined) {
-      return dateTimeResult
-    }
-
-    return undefined
-  }
-
   private escapeRegExpSpecialCharacters(text: string): string {
     return text.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')
   }

test/unit/interpreter/function-value.spec.ts

@@ -83,7 +83,7 @@ describe('Function VALUE', () => {
     it('should convert string with thousand separator', () => {
       const engine = HyperFormula.buildFromArray([
         ['=VALUE("1,234")'],
-      ])
+      ], {thousandSeparator: ',', functionArgSeparator: ';'})
 
       expect(engine.getCellValue(adr('A1'))).toBe(1234)
     })
@@ -95,6 +95,22 @@ describe('Function VALUE', () => {
 
       expect(engine.getCellValue(adr('A1'))).toBe(-123)
     })
+
+    it('should return VALUE error for nested parentheses', () => {
+      const engine = HyperFormula.buildFromArray([
+        ['=VALUE("((123))")'],
+      ])
+
+      expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.VALUE, ErrorMessage.NumberCoercion))
+    })
+
+    it('should return VALUE error for unbalanced parentheses', () => {
+      const engine = HyperFormula.buildFromArray([
+        ['=VALUE("(123")'],
+      ])
+
+      expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.VALUE, ErrorMessage.NumberCoercion))
+    })
   })
 
   describe('percentage strings', () => {
@@ -119,7 +135,7 @@ describe('Function VALUE', () => {
     it('should convert currency string with thousand separator and decimal', () => {
       const engine = HyperFormula.buildFromArray([
         ['=VALUE("$1,234.56")'],
-      ])
+      ], {thousandSeparator: ',', functionArgSeparator: ';'})
 
       expect(engine.getCellValue(adr('A1'))).toBe(1234.56)
     })
@@ -204,12 +220,15 @@ describe('Function VALUE', () => {
       expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.VALUE, ErrorMessage.NumberCoercion))
     })
 
-    it('should return VALUE error for 12-hour time format without proper config', () => {
+  })
+
+  describe('12-hour time format', () => {
+    it('should parse 12-hour time format with am/pm', () => {
       const engine = HyperFormula.buildFromArray([
         ['=VALUE("3:00pm")'],
       ])
 
-      expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.VALUE, ErrorMessage.NumberCoercion))
+      expect(engine.getCellValue(adr('A1'))).toBeCloseTo(0.625, 6)
     })
   })
 
@@ -238,9 +257,17 @@ describe('Function VALUE', () => {
       expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.DIV_BY_ZERO))
     })
 
-    it('should convert cell reference with numeric string', () => {
+    it('should convert cell reference with string value', () => {
+      const engine = HyperFormula.buildFromArray([
+        ["'123", '=VALUE(A1)'],
+      ])
+
+      expect(engine.getCellValue(adr('B1'))).toBe(123)
+    })
+
+    it('should pass through cell reference with numeric value', () => {
       const engine = HyperFormula.buildFromArray([
-        ['123', '=VALUE(A1)'],
+        [123, '=VALUE(A1)'],
       ])
 
       expect(engine.getCellValue(adr('B1'))).toBe(123)
@@ -251,15 +278,15 @@ describe('Function VALUE', () => {
     it('should respect decimal separator config', () => {
       const engine = HyperFormula.buildFromArray([
         ['=VALUE("123,45")'],
-      ], {decimalSeparator: ',', thousandSeparator: ' '})
+      ], {decimalSeparator: ',', thousandSeparator: ' ', functionArgSeparator: ';'})
 
       expect(engine.getCellValue(adr('A1'))).toBe(123.45)
     })
 
     it('should respect thousand separator config', () => {
       const engine = HyperFormula.buildFromArray([
         ['=VALUE("1 234,56")'],
-      ], {decimalSeparator: ',', thousandSeparator: ' '})
+      ], {decimalSeparator: ',', thousandSeparator: ' ', functionArgSeparator: ';'})
 
       expect(engine.getCellValue(adr('A1'))).toBe(1234.56)
     })