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
Fix duplicated messages about deprecated options usage #882
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #882 +/- ##
==========================================
Coverage 92.14% 92.14%
==========================================
Files 167 167
Lines 40515 40530 +15
Branches 5594 3734 -1860
==========================================
+ Hits 37332 37347 +15
- Misses 3142 3177 +35
+ Partials 41 6 -35
|
// eslint-disable-next-line @typescript-eslint/ban-ts-ignore | ||
// @ts-ignore | ||
this.binarySearchThreshold = undefined | ||
this.binarySearchThreshold = binarySearchThreshold ?? Config.defaultConfig.binarySearchThreshold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I am terribly mistaken, this PR missed the most important point, that binarySearchThreshold
is not used anywhere.
Therefore, it should be removed:
- from members of this class
- from the default config object
- from
docs/guide/performance.md
We can keep the warning if someone tries using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a continuation of the original PR (#830) I tried to focus on fixing the double warnings and covering the change with tests only. To keep the PR as simple as possible with the possibility to release the patch version.
Removing the binarySearchThreshold
option from defaults would generate a new TS definition. I don't know if that wouldn't be a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right that it would be an unnecessary breaking change. With your change, it is possible to release this as a PATCH version.
Context
The PR fixes an issue related to the doubled or tripled printed deprecated warning messages.
After the changes once the Hyperformula is initialized the warn messages are logged only once. The messages are logged again only if the
engine.updateConfig()
method is called and only when the deprecated options are passed.Do prints deprecated warn messages:
HyperFormula.buildEmpty({ binarySearchThreshold: 10 });
engine.updateConfig({ binarySearchThreshold: 10 })
Do not print deprecated warn messages:
engine.updateConfig({ precisionRounding: 10 })
engine.rebuildAndRecalculate()
I had a problem with tests in the browser. It turned out that there is a problem with unregistering Jasmine spies that interfere with other tests. To fix it up I upgraded the Jasmine to the latest version.
The PR solves the problem with Handsontable integration where its Formulas plugin calls the HF
rebuildAndRecalculate
method. The method internally merges the Config object with default values (e.g.binarySearchThreshold: 20
) that once passed in the constructor logs again warning messages about the usage of deprecated options. See handsontable/handsontable#8771.How has this been tested?
I tested the changes locally and I added some tests that cover the fix.
Types of changes
Related issue(s):
Checklist: