Skip to content

Commit

Permalink
Fix duplicated messages about deprecated options usage (#882)
Browse files Browse the repository at this point in the history
The commit 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 the commit upgrades the Jasmine to the latest version.

The change 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.
  • Loading branch information
budnix committed Jan 10, 2022
1 parent 51a3b1c commit c38de67
Show file tree
Hide file tree
Showing 6 changed files with 11,291 additions and 10,844 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Expand Up @@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed
- Fixed double warnings about deprecated configuration options. (#882)

## [1.3.0] - 2021-10-20

### Added
Expand Down
52 changes: 26 additions & 26 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions package.json
Expand Up @@ -109,14 +109,14 @@
"eslint-plugin-license-header": "^0.2.0",
"full-icu": "^1.3.1",
"gpu.js": "2.3.0",
"jasmine": "^3.5.0",
"jasmine": "^4.0.0",
"jest": "^26.1.0",
"jsdom": "^16.2.2",
"karma": "^5.0.5",
"karma-chrome-launcher": "^3.1.0",
"karma-firefox-launcher": "^1.3.0",
"karma-jasmine": "^3.1.1",
"karma-jasmine-html-reporter": "^1.5.3",
"karma-jasmine": "^4.0.1",
"karma-jasmine-html-reporter": "^1.7.0",
"karma-sourcemap-loader": "^0.3.7",
"karma-webpack": "^4.0.2",
"license-checker": "^25.0.1",
Expand Down
35 changes: 21 additions & 14 deletions src/Config.ts
Expand Up @@ -586,8 +586,8 @@ export class Config implements ConfigParams, ParserConfig {
public readonly useWildcards: boolean
public readonly matchWholeCell: boolean

constructor(
{
constructor(options: Partial<ConfigParams> = {}, showDeprecatedWarns: boolean = true) {
const {
accentSensitive,
binarySearchThreshold,
caseSensitive,
Expand Down Expand Up @@ -627,8 +627,12 @@ export class Config implements ConfigParams, ParserConfig {
useColumnIndex,
useRegularExpressions,
useWildcards,
}: Partial<ConfigParams> = {},
) {
} = options

if (showDeprecatedWarns) {
this.warnDeprecatedOptions(options)
}

this.useArrayArithmetic = configValueFromParam(useArrayArithmetic, 'boolean', 'useArrayArithmetic')
this.accentSensitive = configValueFromParam(accentSensitive, 'boolean', 'accentSensitive')
this.caseSensitive = configValueFromParam(caseSensitive, 'boolean', 'caseSensitive')
Expand Down Expand Up @@ -659,9 +663,7 @@ export class Config implements ConfigParams, ParserConfig {
validateNumberToBeAtLeast(this.precisionEpsilon, 'precisionEpsilon', 0)
this.useColumnIndex = configValueFromParam(useColumnIndex, 'boolean', 'useColumnIndex')
this.useStats = configValueFromParam(useStats, 'boolean', 'useStats')
// eslint-disable-next-line @typescript-eslint/ban-ts-ignore
// @ts-ignore
this.binarySearchThreshold = undefined
this.binarySearchThreshold = binarySearchThreshold ?? Config.defaultConfig.binarySearchThreshold
this.parseDateTime = configValueFromParam(parseDateTime, 'function', 'parseDateTime')
this.stringifyDateTime = configValueFromParam(stringifyDateTime, 'function', 'stringifyDateTime')
this.stringifyDuration = configValueFromParam(stringifyDuration, 'function', 'stringifyDuration')
Expand All @@ -687,11 +689,6 @@ export class Config implements ConfigParams, ParserConfig {
}
})
validateNumberToBeAtLeast(this.maxColumns, 'maxColumns', 1)
this.warnDeprecatedIfUsed(binarySearchThreshold, 'binarySearchThreshold', '1.1')
this.warnDeprecatedIfUsed(gpujs, 'gpujs', '1.2')
if (gpuMode !== Config.defaultConfig.gpuMode) {
this.warnDeprecatedIfUsed(gpuMode, 'gpuMode', '1.2')
}

privatePool.set(this, {
licenseKeyValidityState: checkLicenseKeyValidity(this.licenseKey)
Expand Down Expand Up @@ -726,7 +723,18 @@ export class Config implements ConfigParams, ParserConfig {
public mergeConfig(init: Partial<ConfigParams>): Config {
const mergedConfig: ConfigParams = Object.assign({}, this.getConfig(), init)

return new Config(mergedConfig)
this.warnDeprecatedOptions(init)

return new Config(mergedConfig, false)
}

private warnDeprecatedOptions(options: Partial<ConfigParams>) {
this.warnDeprecatedIfUsed(options.binarySearchThreshold, 'binarySearchThreshold', '1.1')
this.warnDeprecatedIfUsed(options.gpujs, 'gpujs', '1.2')

if (options.gpuMode !== Config.defaultConfig.gpuMode) {
this.warnDeprecatedIfUsed(options.gpuMode, 'gpuMode', '1.2')
}
}

private warnDeprecatedIfUsed(inputValue: any, paramName: string, fromVersion: string, replacementName?: string) {
Expand Down Expand Up @@ -756,4 +764,3 @@ function getFullConfigFromPartial(partialConfig: Partial<ConfigParams>): ConfigP
export function getDefaultConfig(): ConfigParams {
return getFullConfigFromPartial({})
}

66 changes: 66 additions & 0 deletions test/config.spec.ts
Expand Up @@ -203,6 +203,72 @@ describe('Config', () => {
expect(() => new Config({nullYear: 100})).not.toThrowError()
expect(() => new Config({nullYear: 101})).toThrowError('Config parameter nullYear should be at most 100')
})

describe('deprecated option warning messages', () => {
beforeEach(() => {
spyOn(console, 'warn')
})

afterEach(() => {
try {
// eslint-disable-next-line
// @ts-ignore
console.warn.mockClear() // clears mock in Jest env
} catch {
// eslint-disable-next-line
// @ts-ignore
console.warn.calls.reset() // clears mock in Jasmine env
}
})

it('should log usage of deprecated options when they are passed while engine initialization', () => {
new Config({
binarySearchThreshold: 20,
gpujs: true,
gpuMode: 'gpu',
})

expect(console.warn).toHaveBeenCalledWith('binarySearchThreshold option is deprecated since 1.1')
expect(console.warn).toHaveBeenCalledWith('gpujs option is deprecated since 1.2')
expect(console.warn).toHaveBeenCalledWith('gpuMode option is deprecated since 1.2')
expect(console.warn).toHaveBeenCalledTimes(3)
})

it('should log usage of deprecated options when they are passed while merging the Config object', () => {
const config = new Config()

config.mergeConfig({
binarySearchThreshold: 20,
gpujs: true,
})

expect(console.warn).toHaveBeenCalledTimes(2)
expect(console.warn).toHaveBeenCalledWith('binarySearchThreshold option is deprecated since 1.1')
expect(console.warn).toHaveBeenCalledWith('gpujs option is deprecated since 1.2')
})

it('should not log usage of deprecated options when they are not passed while merging the Config object', () => {
const config = new Config({
binarySearchThreshold: 20,
gpujs: true,
gpuMode: 'gpu',
})

try {
// eslint-disable-next-line
// @ts-ignore
console.warn.mockClear() // clears mock in Jest env
} catch {
// eslint-disable-next-line
// @ts-ignore
console.warn.calls.reset() // clears mock in Jasmine env
}

config.mergeConfig({})

expect(console.warn).toHaveBeenCalledTimes(0)
})
})
})

describe('getConfig', () => {
Expand Down

0 comments on commit c38de67

Please sign in to comment.