Pu/issue617 #619

izulin · 2021-03-29T15:33:30Z

Context

Makes it so raw value used for initialization is stored together with parsed value in the graph.
Raw value is produced when engine is asked for serialization.
Fixes issue #617 by making it so ' is not dropped when provided as an input to the engine.

How has this been tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature or improvement (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Additional language file or change to the existing one (translations)

Related issue(s):

Unwanted string slice when cell value starts with ' #617

Checklist:

My code follows the code style of this project,
My change requires a change to the documentation,
I described the modification in the CHANGELOG.md file.

codecov · 2021-03-29T16:04:08Z

Codecov Report

Merging #619 (cebeeca) into develop (82c0053) will increase coverage by 0.00%.
The diff coverage is 91.66%.

@@           Coverage Diff            @@
##           develop     #619   +/-   ##
========================================
  Coverage    92.45%   92.45%           
========================================
  Files          164      164           
  Lines        38428    38460   +32     
  Branches      5330     5337    +7     
========================================
+ Hits         35527    35559   +32     
  Misses        2864     2864           
  Partials        37       37

Impacted Files	Coverage Δ
src/CellContentParser.ts	`67.75% <0.00%> (+0.73%)`	⬆️
src/GraphBuilder.ts	`77.50% <25.00%> (ø)`
src/Operations.ts	`91.15% <90.90%> (ø)`
...c/DependencyGraph/AddressMapping/AddressMapping.ts	`82.03% <92.30%> (+0.59%)`	⬆️
src/DependencyGraph/ValueCellVertex.ts	`95.45% <94.73%> (-4.55%)`	⬇️
src/ClipboardOperations.ts	`93.75% <100.00%> (+0.08%)`	⬆️
src/DependencyGraph/DependencyGraph.ts	`85.72% <100.00%> (+0.07%)`	⬆️
src/DependencyGraph/MatrixVertex.ts	`94.63% <100.00%> (+0.18%)`	⬆️
src/HyperFormula.ts	`99.00% <100.00%> (ø)`
src/Serialization.ts	`94.63% <100.00%> (+0.71%)`	⬆️

as per the `.tgz` filename change, updates hyperformula to the version from this PR: handsontable/hyperformula#619

test/interpreter/function-lcm.spec.ts

test/interpreter/function-multinomial.spec.ts

test/interpreter/function-gcd.spec.ts

test/interpreter/operator-plus.spec.ts

test/escaping-support.spec.ts

CHANGELOG.md

test/interpreter/criterion-computations.spec.ts

izulin · 2021-03-31T11:59:59Z

@wojciechczerniak

When you type into cell '4 in XL/GS, ' is stripped at the level of UI. Excel engine does not receive ', it just marks the cell somehow. In our case (see linked issue):
when cell is set value with ', we have to store this value in the cell, so when asked for the value we return correct value.
Thus when cell is assigned '4, it's value is the string not the number, and the string is "'4".

test/escaping-support.spec.ts

wojciechczerniak · 2021-03-31T13:00:51Z

When you type into cell '4 in XL/GS, ' is stripped at the level of UI. Excel engine does not receive ', it just marks the cell somehow. In our case (see linked issue):

@izulin Same with date format, currency or percent sign, yet, we keep them. HyperFormula does not have UI, exported content from the Excel should be correctly calculated without altering the data and serialized without loosing any parts of it.

And it's not stripped, Excel keeps the info and it will be shown in the formula bar as the cell content:

Same for Google Sheets.

izulin · 2021-03-31T13:03:18Z

When you type into cell '4 in XL/GS, ' is stripped at the level of UI. Excel engine does not receive ', it just marks the cell somehow. In our case (see linked issue):

@izulin Same with date format, currency or percent sign, yet, we keep them. HyperFormula does not have UI, exported content from the Excel should be correctly calculated without altering the data and serialized without loosing any parts of it.

And it's not stripped, Excel keeps the info and it will be shown in the formula bar as the cell content:

Same for Google Sheets.

Try accessing the cell value, e.g. type 'abcd and do a concatenation on it, or indexing it.

izulin · 2021-03-31T13:18:13Z

Try accessing the cell value, e.g. type 'abcd and do a concatenation on it, or indexing it.

wojciechczerniak · 2021-04-01T11:01:36Z

Try accessing the cell value, e.g. type 'abcd and do a concatenation on it, or indexing it.

So? getCellValue strips the ' for calculations / coercing to text. =getCellValue(A1) & getCellValue(B1).

This is exactly what we need. For rendering the cell we call getCellValue and want it without the escape character.

BUT when we ask for the source with getCellSerialized it should be still there because it is the source data.

leading ' should probably also get omitted when referencing the cell in a formula

jest.config.js

CHANGELOG.md

izulin · 2021-04-03T12:09:54Z

@wojciechczerniak Ok, lets see how you like this approach.

wojciechczerniak

The outcome is what we're looking for. But maybe it would be more useful to have internal getSerializedValue or getRawValue method instead literal arg?

I think we should look at serialization again. It does not work as intended. With getSheetSerialized I should be able to store and recreate the engine.

Consider an example:

User loads the app: initialize engine with data from server
User logs out / saves data: send getAllSheetsSerialized to server and store in databse / file
User logs in again, reinitialize the engine with data from server
Data was lost in the process 💣 💥

it('should not loose sheet information on serialziation', () => {
    const engine1 = HyperFormula.buildFromArray([
      [1, '2', 'foo', true, '\'1', '33$', '12/01/15']
    ])


    expect(engine1.getCellSerialized(adr('A1'))).toEqual(1)
    expect(engine1.getCellValueFormat(adr('A1'))).toEqual(undefined)
    expect(engine1.getCellValueDetailedType(adr('A1'))).toEqual(CellValueDetailedType.NUMBER_RAW)

    expect(engine1.getCellSerialized(adr('B1'))).toEqual(2)
    expect(engine1.getCellValueFormat(adr('B1'))).toEqual(undefined)
    expect(engine1.getCellValueDetailedType(adr('B1'))).toEqual(CellValueDetailedType.NUMBER_RAW)

    expect(engine1.getCellSerialized(adr('C1'))).toEqual('foo')
    expect(engine1.getCellValueFormat(adr('C1'))).toEqual(undefined)
    expect(engine1.getCellValueDetailedType(adr('C1'))).toEqual(CellValueDetailedType.STRING)

    expect(engine1.getCellSerialized(adr('D1'))).toEqual(true)
    expect(engine1.getCellValueFormat(adr('D1'))).toEqual(undefined)
    expect(engine1.getCellValueDetailedType(adr('D1'))).toEqual(CellValueDetailedType.BOOLEAN)

    expect(engine1.getCellSerialized(adr('E1'))).toEqual('\'1')
    expect(engine1.getCellValueFormat(adr('E1'))).toEqual(undefined)
    expect(engine1.getCellValueDetailedType(adr('E1'))).toEqual(CellValueDetailedType.STRING)

    expect(engine1.getCellSerialized(adr('F1'))).toEqual(33)
    expect(engine1.getCellValueFormat(adr('F1'))).toEqual('$')
    expect(engine1.getCellValueDetailedType(adr('F1'))).toEqual(CellValueDetailedType.NUMBER_CURRENCY)

    expect(engine1.getCellSerialized(adr('G1'))).toEqual(42016)
    expect(engine1.getCellValueFormat(adr('G1'))).toEqual(undefined)
    expect(engine1.getCellValueDetailedType(adr('G1'))).toEqual(CellValueDetailedType.NUMBER_DATE)

    // serialize and "send" data to server
    const serialized = engine1.getAllSheetsSerialized();

    // reload data and "restore" the previous state
    const engine2 = HyperFormula.buildFromSheets(serialized)

    expect(engine2.getCellSerialized(adr('A1'))).toEqual(1)
    expect(engine2.getCellValueFormat(adr('A1'))).toEqual(undefined)
    expect(engine2.getCellValueDetailedType(adr('A1'))).toEqual(CellValueDetailedType.NUMBER_RAW)

    expect(engine2.getCellSerialized(adr('B1'))).toEqual(2)
    expect(engine2.getCellValueFormat(adr('B1'))).toEqual(undefined)
    expect(engine2.getCellValueDetailedType(adr('B1'))).toEqual(CellValueDetailedType.NUMBER_RAW)

    expect(engine2.getCellSerialized(adr('C1'))).toEqual('foo')
    expect(engine2.getCellValueFormat(adr('C1'))).toEqual(undefined)
    expect(engine2.getCellValueDetailedType(adr('C1'))).toEqual(CellValueDetailedType.STRING)

    expect(engine2.getCellSerialized(adr('D1'))).toEqual(true)
    expect(engine2.getCellValueFormat(adr('D1'))).toEqual(undefined)
    expect(engine2.getCellValueDetailedType(adr('D1'))).toEqual(CellValueDetailedType.BOOLEAN)

    expect(engine2.getCellSerialized(adr('E1'))).toEqual('\'1')
    expect(engine2.getCellValueFormat(adr('E1'))).toEqual(undefined)
    expect(engine2.getCellValueDetailedType(adr('E1'))).toEqual(CellValueDetailedType.STRING)

    expect(engine2.getCellSerialized(adr('F1'))).toEqual(33)
    expect(engine2.getCellValueFormat(adr('F1'))).toEqual('$')
    expect(engine2.getCellValueDetailedType(adr('F1'))).toEqual(CellValueDetailedType.NUMBER_CURRENCY)

    expect(engine2.getCellSerialized(adr('G1'))).toEqual(42016)
    expect(engine2.getCellValueFormat(adr('G1'))).toEqual(undefined)
    expect(engine2.getCellValueDetailedType(adr('G1'))).toEqual(CellValueDetailedType.NUMBER_DATE)
  })

BTW. does the getCellValueFormat work for NUMBER_DATE?

I think that we should a better serialization approach for all the value types, not only the escaped string

wojciechczerniak · 2021-04-06T14:50:37Z

Don't get me wrong! This is a good solution for the issue at hand. But I think we're going in the wrong direction overall and we will create a technical debt while we can introduce getSerializedValue at cell vertex and handle all types together instead a single if condition for escaped string. WDYT?

izulin · 2021-04-07T10:37:19Z

Don't get me wrong! This is a good solution for the issue at hand. But I think we're going in the wrong direction overall and we will create a technical debt while we can introduce getSerializedValue at cell vertex and handle all types together instead a single if condition for escaped string. WDYT?

Good catch.

I agree this should be handled in a more integrated form.

…rmula into pu/issue617

izulin · 2021-04-08T16:02:27Z

@wojciechczerniak This actually induced a larger refactor (yay CRUDS, yay Matrices!)

lgtm-com · 2021-04-08T16:15:14Z

This pull request introduces 6 alerts when merging 706ae9c into e6cf049 - view on LGTM.com

new alerts:

6 for Unused variable, import, function or class

CHANGELOG.md

lgtm-com · 2021-04-08T18:52:35Z

This pull request introduces 6 alerts when merging 45635e0 into e6cf049 - view on LGTM.com

new alerts:

6 for Unused variable, import, function or class

lgtm-com · 2021-04-09T14:13:16Z

This pull request introduces 6 alerts when merging a57488b into 82c0053 - view on LGTM.com

new alerts:

6 for Unused variable, import, function or class

izulin · 2021-04-12T11:22:07Z

Known issue: raw value is lost if the numeric input becomes part of a matrix.

lgtm-com · 2021-04-12T11:28:04Z

This pull request introduces 8 alerts when merging ab20ae8 into 82c0053 - view on LGTM.com

new alerts:

8 for Unused variable, import, function or class

wojciechczerniak

Awesome! 🍻

Just few cosmetic questions if you don't mind

wojciechczerniak · 2021-04-14T08:49:34Z

test/serialization.spec.ts

+    expect(engine1.getCellValueDetailedType(adr('F1'))).toEqual(CellValueDetailedType.NUMBER_CURRENCY)
+
+    expect(engine1.getCellSerialized(adr('G1'))).toEqual('12/01/15')
+    expect(engine1.getCellValueFormat(adr('G1'))).toEqual(undefined)


Can we add a note that this is wrong? With link to #626

wojciechczerniak · 2021-04-14T08:50:51Z

test/serialization.spec.ts

+describe('serialization', () => {
+  it('should not loose sheet information on serialization', () => {
+    const engine1 = HyperFormula.buildFromArray([
+      [1, '2', 'foo', true, '\'1', '33$', '12/01/15']


Should we add % and a literal error #DIV/0, =#CUSTOM! so we have the whole picture here?

wojciechczerniak · 2021-04-14T08:53:13Z

src/HyperFormula.ts

@@ -643,7 +643,7 @@ export class HyperFormula implements TypedEmitter {
   *
   * @category Cells
   */
-  public getCellSerialized(cellAddress: SimpleCellAddress): NoErrorCellValue {
+  public getCellSerialized(cellAddress: SimpleCellAddress): RawCellContent {


Should we consider this as a breaking change to the public API? IMO yes.

lgtm-com · 2021-04-14T09:18:53Z

This pull request introduces 8 alerts when merging cebeeca into 82c0053 - view on LGTM.com

new alerts:

8 for Unused variable, import, function or class

wojciechczerniak

Thanks!

izulin added 2 commits March 29, 2021 15:55

parsing of escape character

7f80262

test

e1b6746

izulin requested a review from wojciechczerniak March 29, 2021 15:33

doc

2e44288

izulin requested a review from rsify March 29, 2021 16:01

rsify previously approved these changes Mar 30, 2021

View reviewed changes

rsify added a commit to handsontable/handsontable that referenced this pull request Mar 30, 2021

fix escaping with ' prefix

02d3109

as per the `.tgz` filename change, updates hyperformula to the version from this PR: handsontable/hyperformula#619

wojciechczerniak suggested changes Mar 31, 2021

View reviewed changes

izulin commented Mar 31, 2021

View reviewed changes

test/escaping-support.spec.ts Outdated Show resolved Hide resolved

Update test/escaping-support.spec.ts

766296e

proper distinction between serialization and value

4f99654

izulin commented Apr 3, 2021

View reviewed changes

jest.config.js Outdated Show resolved Hide resolved

Update jest.config.js

4ae0ed0

izulin commented Apr 3, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

99cb1b5

izulin requested a review from wojciechczerniak April 3, 2021 12:09

wojciechczerniak suggested changes Apr 6, 2021

View reviewed changes

Merge branch 'develop' into pu/issue617

d8e89a7

izulin and others added 2 commits April 7, 2021 16:20

Merge branch 'develop' into pu/issue617

91af653

refactor for preserving original raw value

45a05c2

Merge branch 'pu/issue617' of https://github.com/handsontable/hyperfo…

3714bf1

…rmula into pu/issue617

izulin requested a review from wojciechczerniak April 8, 2021 16:01

minor issue

706ae9c

izulin commented Apr 8, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

45635e0

izulin requested a review from voodoo11 April 8, 2021 18:50

Merge branch 'develop' into pu/issue617

a57488b

izulin added 2 commits April 12, 2021 13:17

small refactor

9c46248

linter

ab20ae8

wojciechczerniak suggested changes Apr 14, 2021

View reviewed changes

izulin added 2 commits April 14, 2021 11:10

fixes to tests

b749c7f

doc

cebeeca

izulin requested a review from wojciechczerniak April 14, 2021 09:13

wojciechczerniak approved these changes Apr 14, 2021

View reviewed changes

izulin merged commit 67d5802 into develop Apr 14, 2021

izulin deleted the pu/issue617 branch April 14, 2021 19:03

izulin mentioned this pull request Apr 21, 2021

Unwanted string slice when cell value starts with ' #617

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pu/issue617 #619

Pu/issue617 #619

izulin commented Mar 29, 2021 •

edited

Loading

codecov bot commented Mar 29, 2021 •

edited

Loading

izulin commented Mar 31, 2021

wojciechczerniak commented Mar 31, 2021

izulin commented Mar 31, 2021

izulin commented Mar 31, 2021

wojciechczerniak commented Apr 1, 2021

izulin commented Apr 3, 2021

wojciechczerniak left a comment •

edited

Loading

wojciechczerniak commented Apr 6, 2021

izulin commented Apr 7, 2021

izulin commented Apr 8, 2021

lgtm-com bot commented Apr 8, 2021

lgtm-com bot commented Apr 8, 2021

lgtm-com bot commented Apr 9, 2021

izulin commented Apr 12, 2021

lgtm-com bot commented Apr 12, 2021

wojciechczerniak left a comment

wojciechczerniak Apr 14, 2021

izulin Apr 14, 2021

wojciechczerniak Apr 14, 2021

izulin Apr 14, 2021

wojciechczerniak Apr 14, 2021

izulin Apr 14, 2021

lgtm-com bot commented Apr 14, 2021

wojciechczerniak left a comment

Pu/issue617 #619

Pu/issue617 #619

Conversation

izulin commented Mar 29, 2021 • edited Loading

Context

How has this been tested?

Types of changes

Related issue(s):

Checklist:

codecov bot commented Mar 29, 2021 • edited Loading

Codecov Report

izulin commented Mar 31, 2021

wojciechczerniak commented Mar 31, 2021

izulin commented Mar 31, 2021

izulin commented Mar 31, 2021

wojciechczerniak commented Apr 1, 2021

izulin commented Apr 3, 2021

wojciechczerniak left a comment • edited Loading

Choose a reason for hiding this comment

wojciechczerniak commented Apr 6, 2021

izulin commented Apr 7, 2021

izulin commented Apr 8, 2021

lgtm-com bot commented Apr 8, 2021

lgtm-com bot commented Apr 8, 2021

lgtm-com bot commented Apr 9, 2021

izulin commented Apr 12, 2021

lgtm-com bot commented Apr 12, 2021

wojciechczerniak left a comment

Choose a reason for hiding this comment

wojciechczerniak Apr 14, 2021

Choose a reason for hiding this comment

izulin Apr 14, 2021

Choose a reason for hiding this comment

wojciechczerniak Apr 14, 2021

Choose a reason for hiding this comment

izulin Apr 14, 2021

Choose a reason for hiding this comment

wojciechczerniak Apr 14, 2021

Choose a reason for hiding this comment

izulin Apr 14, 2021

Choose a reason for hiding this comment

lgtm-com bot commented Apr 14, 2021

wojciechczerniak left a comment

Choose a reason for hiding this comment

izulin commented Mar 29, 2021 •

edited

Loading

codecov bot commented Mar 29, 2021 •

edited

Loading

wojciechczerniak left a comment •

edited

Loading