Pu/typing #590

izulin · 2020-12-28T12:37:04Z

Context

Related with #313

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):

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.

lgtm-com · 2020-12-28T12:43:11Z

This pull request introduces 1 alert when merging bcf1d7f into 074831e - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

codecov · 2020-12-28T12:52:07Z

Codecov Report

Merging #590 (2198e1d) into develop (8befe02) will decrease coverage by 0.19%.
The diff coverage is 86.60%.

@@             Coverage Diff             @@
##           develop     #590      +/-   ##
===========================================
- Coverage    92.70%   92.50%   -0.20%     
===========================================
  Files          162      163       +1     
  Lines        37840    38342     +502     
  Branches      5256     5314      +58     
===========================================
+ Hits         35078    35468     +390     
- Misses        2756     2868     +112     
  Partials         6        6

Impacted Files	Coverage Δ
src/DateTimeHelper.ts	`82.50% <13.33%> (-2.38%)`	⬇️
src/CellContentParser.ts	`67.02% <30.95%> (-9.33%)`	⬇️
src/GraphBuilder.ts	`77.50% <50.00%> (+0.09%)`	⬆️
src/interpreter/binarySearch.ts	`74.44% <50.00%> (-10.18%)`	⬇️
src/interpreter/plugin/FunctionPlugin.ts	`83.75% <70.31%> (-0.52%)`	⬇️
src/interpreter/ArithmeticHelper.ts	`84.79% <74.83%> (-3.20%)`	⬇️
src/Operations.ts	`91.27% <77.77%> (+<0.01%)`	⬆️
src/Config.ts	`96.18% <84.00%> (-0.45%)`	⬇️
src/Exporter.ts	`86.44% <85.71%> (-0.23%)`	⬇️
src/interpreter/SimpleRangeValue.ts	`87.05% <87.05%> (ø)`
... and 72 more

lgtm-com · 2020-12-28T18:55:21Z

This pull request introduces 1 alert when merging 8f481a3 into 074831e - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

lgtm-com · 2020-12-29T10:08:09Z

This pull request introduces 1 alert when merging 3767279 into 074831e - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

krzysztofspilka · 2020-12-29T14:46:23Z

@izulin what about is this PR?

izulin · 2020-12-29T14:47:46Z

@izulin what about is this PR?

It is a WIP for #313

lgtm-com · 2021-01-08T18:27:56Z

This pull request introduces 3 alerts when merging 782a2cf into 6870a3a - view on LGTM.com

new alerts:

3 for Unused variable, import, function or class

wojciechczerniak

Sorry for the late review

Have you checked how types are affecting our benchmarks?
We call getRawValue a lot. Isn't there a single place to do that?

I will test some more later

src/interpreter/plugin/InformationPlugin.ts

src/index.ts

wojciechczerniak · 2021-01-10T22:30:30Z

src/Config.ts

+  /**
+   * Symbol used to denote currency numbers.
+   *
+   * @default '$'
+   *
+   * @category Number
+   */
+  currencySymbol: string,


Isn't this an oversimplification? Each cell can have a different currency, especially with currency conversion functions. I'm not sure a single symbol is enough. We should have something similar to parseDate callback but with row/column so each cell can be parsed differently? I think we've discussed this before #314

GS accepts only one currency symbol at a given moment in the whole spreadsheet.

Can you elaborate on that? I see more

Ok, I see. What do you propose then?

This is tricky. Currency is different than Date (which is parsed from a string / on the editor entry). As we did it, parsing "$20" to currency from string, is different than what GS or XL does. They store some kind of metadata with the format. Value is the formula bar is unchanged numeric:

And entered "$20" is a string. We will have incompatibility here with all the spreadsheets if we keep parsing it to CurrencyType. What we're tracking here is the number formatting, ie. date can have two different formats and they are still passed through the formula:

What you did is great to track the Type and set the correct cell editor (number/date/text) in the Handsontable, that is what @jansiegel asked for.

Internally should be doable. We can save the format on CurrenctyType and DateType as an object property. And when the type is requested with API both will be available (DateType and DateType.format). This is rather an extension of what we have here.

The problem is how to set the Type and/or Format. Because all we've got is an array of simple types and no metadata: ['1', '1%', '1$', '01/01/1900', '12:00', '01/01/1900 12:00']. Guess our problem is how to get/set formats from that input. Where:

1 is NumericType

1% is PercentType

1$ is StringType (for all spreadsheets 💣 )

01/01/1900 is DateType, but... 01 01 1900, 01 Jan 1900, 01 January 1900 and others are also recognized 🙈

so there is no automatic recognition of the CurrencyType. It's a number with formatting. But take a look at the date type, it's a DateType with different formatting. I guess number, format is also tracked. Try setting scientific at A1 and then add other formats to it =A1+B1 etc. The first cell (in the formula) format is applied to the result.

Excel has a lot more info, and the CELL function is proof of that: https://support.microsoft.com/en-us/office/cell-function-51bd39a5-f338-4dbe-a33f-955d67c2b2cf with all the properties you can request for a cell.

Discussed and resolved in #599.

src/HyperFormula.ts

wojciechczerniak · 2021-01-18T18:33:36Z

src/interpreter/plugin/FunctionPlugin.ts

+  /**
+   * Argument should be passed with full type information.
+   * (e.g. Date/DateTime/Time/Currency/Percentage for numbers)
+   */
+  passSubtype?: boolean,


Do we have to mark it in function metadata? If we know that the argument has a returnNumberType set, we may assume that it should be passed as a full type?

If we know that the argument has a returnNumberType set, we may assume that it should be passed as a full type?

Not really. Most functions ignore input types. Some functions (usualy logical ones) use the type, e.g. IF etc.
Some other function set output type (e.g. financial).

So shouldn't this be up to the function to decide?
I believe it already does but based on some metadata to the Core, which may be confusing and adds to the complexity of custom functions

So shouldn't this be up to the function to decide?

Why not keep it in metadata, since it is a pretty automatic decision (you either want raw data, or rich data).

I don't have better arguments. Metadata is fine.

src/interpreter/plugin/FunctionPlugin.ts

src/interpreter/ArithmeticHelper.ts

test/type-inference.spec.ts

Co-authored-by: Wojciech Czerniak <wojciech.czerniak@gmail.com>

izulin · 2021-01-18T22:32:45Z

Sorry for the late review

Have you checked how types are affecting our benchmarks?

@voodoo11 benchmarked

We call getRawValue a lot. Isn't there a single place to do that?

Not to my knowledge.

I will test some more later

Co-authored-by: Wojciech Czerniak <wojciech.czerniak@gmail.com>

lgtm-com · 2021-01-18T22:44:04Z

This pull request introduces 3 alerts when merging 6c73576 into 6870a3a - view on LGTM.com

new alerts:

3 for Unused variable, import, function or class

src/interpreter/ArithmeticHelper.ts

lgtm-com · 2021-01-19T15:18:08Z

This pull request introduces 3 alerts when merging 90091b5 into 6870a3a - view on LGTM.com

new alerts:

3 for Unused variable, import, function or class

lgtm-com · 2021-01-22T15:07:17Z

This pull request introduces 3 alerts when merging 06fe4d8 into 6870a3a - view on LGTM.com

new alerts:

3 for Unused variable, import, function or class

wojciechczerniak · 2021-01-26T18:44:48Z

Sorry for the late review

Have you checked how types are affecting our benchmarks?

@voodoo11 benchmarked

And what are the results, any difference?

We call getRawValue a lot. Isn't there a single place to do that?

Not to my knowledge.

Ok

voodoo11 · 2021-01-27T15:22:14Z

And what are the results, any difference?

Evaluation slowed down by 20% average, but overall build time increased only by 5% at most.

bardek8 · 2021-01-30T22:53:35Z

We can also change Known limitations file as Currency data type will be now supported (though with some limitations).

(btw also complex numbers are now supported :) )

* config * extra val * passes tests * linter * refactor * linter * docs * exports * Update src/HyperFormula.ts Co-authored-by: Wojciech Czerniak <wojciech.czerniak@gmail.com> * docs * Update src/HyperFormula.ts Co-authored-by: Wojciech Czerniak <wojciech.czerniak@gmail.com> * refactor * . * bug * linter Co-authored-by: Wojciech Czerniak <wojciech.czerniak@gmail.com>

lgtm-com · 2021-02-01T17:09:37Z

This pull request introduces 6 alerts when merging bdc742d into 4c318ea - view on LGTM.com

new alerts:

5 for Unused variable, import, function or class
1 for Useless assignment to local variable

lgtm-com · 2021-02-01T17:17:03Z

This pull request introduces 3 alerts when merging 625abcd into 4c318ea - view on LGTM.com

new alerts:

3 for Unused variable, import, function or class

wojciechczerniak

Cool! I think we're good here 🏆 The documentation could be improved though:

Here: https://handsontable.github.io/hyperformula/guide/types-of-values.html we should add Currency and Percent types to the list, and after sections about getCellType and getCellValueType we should add two sections for the new methods: getCellValueFormat and getCellValueDetailedType
the new returnNumberType and passSubtype properties should have a mention on https://handsontable.github.io/hyperformula/guide/custom-functions.html page

wojciechczerniak · 2021-02-01T22:18:17Z

CHANGELOG.md

@@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ### Added
 - Added support for row and column reordering. (#343)
+- Added type inferrence for subtypes for number. (#313)
+- Added parsing of number literals containing '%' or currency symbol (default '$').


Please add the issue/PR number

we're not really explaining many aspects of metadata, e.g. automatic coercions of arguments. The missing part is part of runFunction wrapper, which is not mandatory for usage. The same for returnNumberType and passSubtype -- those are from runFunction wrapper.

Yup. We have an issue #557 to update the docs after the runFunction was introduced

lgtm-com · 2021-02-02T11:43:49Z

This pull request introduces 3 alerts when merging 2198e1d into 8befe02 - view on LGTM.com

new alerts:

3 for Unused variable, import, function or class

izulin added 19 commits December 18, 2020 12:52

refactor

b299263

linter

90f0791

function plugin

cfc415f

typing

616a172

more fixes

b431e3a

more fixes

c61681e

typing

28c0bfe

plugins done

fb36918

more typing, templates

d5bb7f4

.

f44ace5

.

2b412da

major refactor

5146b70

.

457a3c5

.

b60024f

compiles!

2e2386f

tests passed

8bd49da

linter

425bf0b

imports

f59b758

linter

bcf1d7f

izulin marked this pull request as draft December 28, 2020 12:37

izulin and others added 2 commits December 28, 2020 19:36

toEqual

64c1d55

Merge branch 'develop' into pu/typing

8f481a3

.

3767279

.

29edf27

typing

782a2cf

wojciechczerniak suggested changes Jan 18, 2021

View reviewed changes

Update src/interpreter/plugin/InformationPlugin.ts

5d296c2

Co-authored-by: Wojciech Czerniak <wojciech.czerniak@gmail.com>

izulin and others added 2 commits January 18, 2021 23:33

Update src/index.ts

4862c82

Co-authored-by: Wojciech Czerniak <wojciech.czerniak@gmail.com>

Update src/HyperFormula.ts

6c73576

Co-authored-by: Wojciech Czerniak <wojciech.czerniak@gmail.com>

izulin commented Jan 19, 2021

View reviewed changes

src/interpreter/ArithmeticHelper.ts Outdated Show resolved Hide resolved

Update src/interpreter/ArithmeticHelper.ts

90091b5

export

06fe4d8

izulin requested a review from wojciechczerniak January 22, 2021 14:59

wojciechczerniak mentioned this pull request Jan 31, 2021

Pu/extratyping #599

Merged

7 tasks

izulin and others added 2 commits February 1, 2021 17:42

Merge branch 'develop' into pu/typing

bdc742d

merge

625abcd

wojciechczerniak approved these changes Feb 1, 2021

View reviewed changes

izulin added 2 commits February 1, 2021 23:53

docs

8af2776

Merge branch 'develop' into pu/typing

2198e1d

izulin merged commit 09aead6 into develop Feb 2, 2021

izulin deleted the pu/typing branch February 2, 2021 12:25

wojciechczerniak mentioned this pull request Mar 16, 2021

Track cell type better #313

Closed

Pu/typing #590

Pu/typing #590

Conversation

izulin commented Dec 28, 2020 • edited

Context

How has this been tested?

Types of changes

Related issue(s):

Checklist:

lgtm-com bot commented Dec 28, 2020

codecov bot commented Dec 28, 2020 • edited

Codecov Report

lgtm-com bot commented Dec 28, 2020

lgtm-com bot commented Dec 29, 2020

krzysztofspilka commented Dec 29, 2020

izulin commented Dec 29, 2020

lgtm-com bot commented Jan 8, 2021

wojciechczerniak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izulin commented Jan 18, 2021

lgtm-com bot commented Jan 18, 2021

lgtm-com bot commented Jan 19, 2021

lgtm-com bot commented Jan 22, 2021

wojciechczerniak commented Jan 26, 2021

voodoo11 commented Jan 27, 2021

bardek8 commented Jan 30, 2021

lgtm-com bot commented Feb 1, 2021

lgtm-com bot commented Feb 1, 2021

wojciechczerniak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Feb 2, 2021

izulin commented Dec 28, 2020 •

edited

codecov bot commented Dec 28, 2020 •

edited