Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More advanced custom function tutorial #1108

Merged
merged 32 commits into from
Dec 20, 2022
Merged

More advanced custom function tutorial #1108

merged 32 commits into from
Dec 20, 2022

Conversation

sequba
Copy link
Contributor

@sequba sequba commented Nov 23, 2022

Context

Updated demo as a PR in demos repo. Works in CodeSandbox (sandbox support for the unit tests works).

  • Update the custom-functions demo to create a simple function that:
    • accepts a string argument
    • returns a string
  • Add another example of a more complex custom function that:
    • performs a mathematical operation on numbers
    • accepts a range argument
    • validates the argument
    • returns an array (see export ArraySize from hyperformula #843)
    • validates the argument
    • is testable (there is an example of a unit test)
  • And:
    • update the custom-functions guide to describe the updated demo
    • rename ArgumentTypes interface to FunctionArgumentType and export it for typescript users

How did you test your changes?

  • unit test pass
  • add unit tests that create custom functions from the demo
  • test demo code
  • Grammarly
  • typescript test
  • local docs build
  • request review from the external expert

Types of changes

  • Breaking change (a fix or a feature because of which an existing functionality doesn't work as expected anymore)
  • New feature or improvement (a non-breaking change that adds functionality)
  • Bug fix (a non-breaking change that fixes an issue)
  • Additional language file, or a change to an existing language file (translations)
  • Change to the documentation

Related issues:

Fixes #779

Checklist:

  • My change is compliant with the OpenDocument standard.
  • My change is compatible with Microsoft Excel.
  • My change is compatible with Google Sheets.
  • My code follows the code style of this project.
  • I described my changes in the CHANGELOG.md file.
  • My changes require a documentation update.
  • My changes require a migration guide.

@github-actions
Copy link

github-actions bot commented Nov 23, 2022

Performance comparison of head (318d094) vs base (aa3f7c0)

                                     testName |  base |  head |  change
-----------------------------------------------------------------------
                                      Sheet A | 458.1 | 462.8 |  +1.03%
                                      Sheet B |   201 | 203.9 |  +1.44%
                                      Sheet T | 242.5 |   243 |  +0.21%
                                Column ranges | 554.2 | 565.1 |  +1.97%
Sheet A:  change value, add/remove row/column |    36 |    32 | -11.11%
 Sheet B: change value, add/remove row/column |   314 |   325 |  +3.50%
                   Column ranges - add column |   212 |   246 | +16.04%
                Column ranges - without batch |   614 |   721 | +17.43%
                        Column ranges - batch |   139 |   154 | +10.79%

@sequba sequba linked an issue Nov 24, 2022 that may be closed by this pull request
5 tasks
src/index.ts Show resolved Hide resolved
Copy link
Member

@warpech warpech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The page is already good, but with large amounts of text it is easy to add a large amount of comments!

src/interpreter/plugin/FunctionPlugin.ts Show resolved Hide resolved
docs/guide/custom-functions.md Outdated Show resolved Hide resolved
docs/guide/custom-functions.md Show resolved Hide resolved
docs/guide/custom-functions.md Outdated Show resolved Hide resolved
docs/guide/custom-functions.md Outdated Show resolved Hide resolved
docs/guide/custom-functions.md Outdated Show resolved Hide resolved
docs/guide/custom-functions.md Outdated Show resolved Hide resolved
docs/guide/custom-functions.md Outdated Show resolved Hide resolved
docs/guide/custom-functions.md Outdated Show resolved Hide resolved
docs/guide/custom-functions.md Outdated Show resolved Hide resolved
@sequba sequba requested a review from warpech December 14, 2022 11:59
Copy link
Member

@warpech warpech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review

docs/guide/custom-functions.md Outdated Show resolved Hide resolved
docs/guide/custom-functions.md Show resolved Hide resolved
Copy link
Member

@warpech warpech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review pt. 2

docs/guide/custom-functions.md Outdated Show resolved Hide resolved
state,
this.metadata('DOUBLE_RANGE'),
(range) => {
const resultArray = //...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it on purpose that the body of the function is omitted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this snippet demonstrates usage of SimpleRangeValue to return an array

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it does not demonstrate that fully, because the body of the function is missing. I think that a snippet is valuable only if it contains working code.

For one, the fully working example added in handsontable/hyperformula-demos#14 uses internalScalarValueToNumber, which is not explained here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion it would unnecessarily increase the complexity of the guide. A reader can see the fully working example in the demo section if interested in the details.

@sequba sequba requested a review from warpech December 16, 2022 14:40
Copy link
Member

@warpech warpech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is the final review from me. I realize that my comments are "yak shaving", but they come from trying to put the guide in practice. My full journey is documented as commits in https://github.com/handsontable/hyperformula-demos/tree/feature/issue-779-recreation-from-guide-in-typescript

Please decide which of these comments must be applied immediately and which can be postponed after the release.


In a more advanced example, we'll create a custom function `DOUBLE_RANGE` that takes a range of numbers and returns the range of the same size with all the numbers doubled.

### Accept a range argument
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm lost, because the guide so far presented just one argument type: STRING. Now, it jumps to RANGE, without answering some questions that popped up in my head:

  1. It is not clear, why only STRING and RANGE types are presented in the guide. Are other types (NUMBER, BOOLEAN, SCALAR, NOERROR, INTEGER, COMPLEX, ANY) more like STRING or more like RANGE or yet totally different?
  2. When should I use RANGE type?

I checked the popularity of all argument types in src. Here are the results:

  • NUMBER - 377
  • RANGE - 63
  • STRING- 46
  • ANY - 33
  • NOERROR - 30
  • COMPLEX - 25
  • INTEGER - 27
  • BOOLEAN - 22
  • SCALAR - 21

So all of them are useful for some types of functions. It would be good to write a sentence about each of them, and why is RANGE special among them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Elaborating more on various argument types would be helpful to a developer of the custom functions. I'll create a follow-up task to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

state,
this.metadata('DOUBLE_RANGE'),
(range) => {
const resultArray = //...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it does not demonstrate that fully, because the body of the function is missing. I think that a snippet is valuable only if it contains working code.

For one, the fully working example added in handsontable/hyperformula-demos#14 uses internalScalarValueToNumber, which is not explained here.

docs/guide/custom-functions.md Outdated Show resolved Hide resolved
this.metadata('DOUBLE_RANGE'),
(range) => {
const resultArray = //...
return SimpleRangeValue.onlyValues(resultArray);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of our built-in functions return SimpleRangeValue.onlyNumbers and SimpleRangeValue.onlyRanges. Might I need them? What's the difference between them and SimpleRangeValue.onlyValues?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a valid question. I'll create a follow-up task to elaborate on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


### Validate the arguments and return an error

To handle invalid inputs, the custom function should return an instance of the [`CellError` class](../api/classes/cellerror.md) with the relevant [error type](types-of-errors.md).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence contradicts with what was said earlier:

To benefit from HyperFormula's automatic validations, wrap your method in the built-in runFunction() method, which: (...) Validates arguments passed to your function against your argument validation options.

Can you give more details about what do I need to validate that is not done by runFunction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll create a follow-up task to elaborate on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs/guide/custom-functions.md Outdated Show resolved Hide resolved
docs/guide/custom-functions.md Outdated Show resolved Hide resolved
docs/guide/custom-functions.md Show resolved Hide resolved
docs/guide/custom-functions.md Show resolved Hide resolved
@sequba sequba mentioned this pull request Dec 20, 2022
5 tasks
@codecov
Copy link

codecov bot commented Dec 20, 2022

Codecov Report

Merging #1108 (318d094) into develop (aa3f7c0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1108   +/-   ##
========================================
  Coverage    95.82%   95.82%           
========================================
  Files          162      162           
  Lines        14091    14097    +6     
  Branches      3004     3004           
========================================
+ Hits         13503    13509    +6     
  Misses         588      588           
Impacted Files Coverage Δ
src/Lookup/AdvancedFind.ts 96.15% <ø> (ø)
src/Lookup/ColumnBinarySearch.ts 100.00% <ø> (ø)
src/Lookup/RowSearchStrategy.ts 100.00% <ø> (ø)
src/Lookup/SearchStrategy.ts 100.00% <ø> (ø)
src/interpreter/CriterionFunctionCompute.ts 98.94% <ø> (ø)
src/interpreter/InterpreterValue.ts 100.00% <ø> (ø)
src/ArraySize.ts 95.41% <100.00%> (ø)
src/ArrayValue.ts 81.17% <100.00%> (ø)
src/Cell.ts 92.45% <100.00%> (ø)
src/ContentChanges.ts 100.00% <100.00%> (ø)
... and 51 more

@sequba sequba merged commit 1696cbc into develop Dec 20, 2022
@sequba sequba deleted the feature/issue-779 branch December 20, 2022 19:18
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.

Docs: add a more advanced custom function tutorial
2 participants