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

Add MINIFS and MAXIFS functions #1050

Merged
merged 21 commits into from
Sep 6, 2022
Merged

Add MINIFS and MAXIFS functions #1050

merged 21 commits into from
Sep 6, 2022

Conversation

sequba
Copy link
Contributor

@sequba sequba commented Aug 22, 2022

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
@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
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.

This is not full review yet, I just checked:

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

@sequba
Copy link
Contributor Author

sequba commented Aug 24, 2022

@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
Copy link

github-actions bot commented Sep 5, 2022

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%

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 reviewed the code and it looks good. Thanks for the code reuse in ConditionalAggregationPlugin.ts!

@codecov
Copy link

codecov bot commented Sep 6, 2022

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