Skip to content

Add MINIFS and MAXIFS functions - #1050

Merged
sequba merged 21 commits into
developfrom
feature/issue-1049
Sep 6, 2022
Merged

Add MINIFS and MAXIFS functions#1050
sequba merged 21 commits into
developfrom
feature/issue-1049

Conversation

@sequba

@sequba sequba commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

Context

@rkretzschmar in #1047 wrote:

  • adds two not implemented functions MINIFS and MAXIFS to the built-in functions
  • adds only English and German translations, all other language files contain the English translation to pass the i18n.spec.ts
  • describes the two new functions in the built-in-functions.md
  • added two new test suites function-minifs.spec.ts and function-maxifs.spec.ts

TODO:

  • check compatibility
    • ODFF (no such function)
    • Excel
    • Google Sheets
  • refactor
    • rename SumifPlugin -> ConditionalAggregationPlugin
    • refactor existing functions inside ConditionalAggregationPlugin
    • define MAXIFS and MINIFS inside ConditionalAggregationPlugin
  • add missing translations
  • don't throw error when taking min/max of zero values
  • test with other data types (currency, date etc)

Co-authored-by: Rene Kretzschmar rkretzschmar@gmail.com

How has this been tested?

Unit tests pass (including new test suites for MINIFS and MAXIFS

Types of changes

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

Related issues:

Fixes #1049
#1047

Checklist:

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

Co-authored-by: Rene Kretzschmar <rkretzschmar@gmail.com>
@sequba sequba linked an issue Aug 22, 2022 that may be closed by this pull request
@sequba sequba changed the title Add MINIFS and MAXIFS functions Add MINIFS and MAXIFS functions (Co-authored-by: Rene Kretzschmar <rkretzschmar@gmail.com>) Aug 22, 2022
@sequba sequba changed the title Add MINIFS and MAXIFS functions (Co-authored-by: Rene Kretzschmar <rkretzschmar@gmail.com>) Add MINIFS and MAXIFS functions Aug 22, 2022
@sequba sequba self-assigned this Aug 22, 2022
@sequba
sequba marked this pull request as ready for review August 23, 2022 11:37
Comment thread src/interpreter/plugin/ConditionalAggregationPlugin.ts
@handsontable handsontable deleted a comment from lgtm-com Bot Aug 23, 2022
@handsontable handsontable deleted a comment from lgtm-com Bot Aug 23, 2022
@sequba
sequba requested a review from warpech August 23, 2022 14:08

@warpech warpech left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not full review yet, I just checked:

  • changelog
  • if the demo in CodeSandbox still works
  • the docs changes in dev preview

Comment thread CHANGELOG.md Outdated
Comment thread docs/guide/built-in-functions.md Outdated
Comment thread docs/guide/built-in-functions.md Outdated
Comment thread docs/guide/built-in-functions.md Outdated
Comment thread src/i18n/languages/deDE.ts
@sequba

sequba commented Aug 24, 2022

Copy link
Copy Markdown
Contributor Author

@rkretzschmar Your general idea for the implementation of these two functions was good. I only made a few changes to comply with the standards of HyperFormula:

  • move the implementation of MAXIFS and MINIFS to different file and refactor it
  • return 0 instead of an error when trying to calculate max/min of an empty range (compatibility with Microsoft Excel)
  • make it ignore empty cells, strings and booleans (compatibility with Microsoft Excel)
  • add unit tests for handling other data types and some edge cases
  • add missing translations
  • rephrase the functions' descriptions in the documentation
  • add changelog entry
  • pull recent changes from develop branch

Once again, thanks for your contribution. You helped us a lot 🔥

@sequba
sequba requested a review from warpech August 24, 2022 14:37
@github-actions

github-actions Bot commented Sep 5, 2022

Copy link
Copy Markdown

Performance comparison of head (c9e5bb6) vs base (06047d2)

                                     testName |  base |  head |  change
-----------------------------------------------------------------------
                                      Sheet A | 644.9 | 614.9 |  -4.65%
                                      Sheet B | 286.1 | 295.8 |  +3.39%
                                      Sheet T | 353.4 | 364.3 |  +3.08%
                                Column ranges |   875 | 869.1 |  -0.67%
Sheet A:  change value, add/remove row/column |    43 |    46 |  +6.98%
 Sheet B: change value, add/remove row/column |   620 |   388 | -37.42%
                   Column ranges - add column |   307 |   263 | -14.33%
                Column ranges - without batch |   924 |  1028 | +11.26%
                        Column ranges - batch |   258 |   192 | -25.58%

@warpech warpech left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I reviewed the code and it looks good. Thanks for the code reuse in ConditionalAggregationPlugin.ts!

@codecov

codecov Bot commented Sep 6, 2022

Copy link
Copy Markdown

Codecov Report

Merging #1050 (c9e5bb6) into develop (06047d2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1050   +/-   ##
========================================
  Coverage    95.74%   95.74%           
========================================
  Files          161      161           
  Lines        14091    14098    +7     
  Branches      2974     2977    +3     
========================================
+ Hits         13491    13498    +7     
  Misses         584      584           
  Partials        16       16           
Impacted Files Coverage Δ
src/i18n/languages/csCZ.ts 100.00% <ø> (ø)
src/i18n/languages/daDK.ts 100.00% <ø> (ø)
src/i18n/languages/deDE.ts 100.00% <ø> (ø)
src/i18n/languages/enGB.ts 100.00% <ø> (ø)
src/i18n/languages/esES.ts 100.00% <ø> (ø)
src/i18n/languages/fiFI.ts 100.00% <ø> (ø)
src/i18n/languages/frFR.ts 100.00% <ø> (ø)
src/i18n/languages/huHU.ts 100.00% <ø> (ø)
src/i18n/languages/itIT.ts 100.00% <ø> (ø)
src/i18n/languages/nbNO.ts 100.00% <ø> (ø)
... and 9 more

@sequba
sequba merged commit 2805b59 into develop Sep 6, 2022
@sequba
sequba deleted the feature/issue-1049 branch September 6, 2022 10:46
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.

MINIFS nad MAXIFS functions

4 participants