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.

Sign up for GitHub

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

Jump to bottom

Fix duplicated messages about deprecated options usage #882

Merged
merged 6 commits into from Jan 10, 2022
Merged

Fix duplicated messages about deprecated options usage #882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
03c0f14
Fix duplicated messages about deprecated options usage
budnix Jan 3, 2022
41e7a02
Fix binarySearchThreshold prop assigning
budnix Jan 4, 2022
cad3460
Change the second argument name of the Config object
budnix Jan 4, 2022
f01649f
Upgrade jasmine to ^4
budnix Jan 4, 2022
f528ad5
Fix tests for Jest env
budnix Jan 4, 2022
e22ff93
Fix lint complains
budnix Jan 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
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
View file
Open in desktop

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
View file
Open in desktop
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
View file
Open in desktop
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
Copy link
Member

@warpech warpech Jan 7, 2022 •
edited

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.

Copy link
Member Author

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?

Copy link
Member

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.

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
View file
Open in desktop
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