Skip to content

Conversation

@izulin
Copy link
Collaborator

@izulin izulin commented Oct 3, 2021

Context

Removes unnecessary warnings.

How has this been tested?

Types of changes

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

Related issue(s):

Checklist:

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

@izulin
Copy link
Collaborator Author

izulin commented Oct 3, 2021

Should fix #828

@codecov
Copy link

codecov bot commented Oct 6, 2021

Codecov Report

Merging #830 (d71dd3b) into develop (0f656c5) will decrease coverage by 0.00%.
The diff coverage is 85.41%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #830      +/-   ##
===========================================
- Coverage    92.43%   92.42%   -0.01%     
===========================================
  Files          167      167              
  Lines        40301    40362      +61     
  Branches      5578     5594      +16     
===========================================
+ Hits         37252    37305      +53     
- Misses        3043     3051       +8     
  Partials         6        6              
Impacted Files Coverage Δ
src/ArraySize.ts 63.85% <50.00%> (-0.12%) ⬇️
src/interpreter/SimpleRangeValue.ts 90.90% <50.00%> (+0.05%) ⬆️
src/DependencyGraph/DependencyGraph.ts 83.15% <55.55%> (-0.25%) ⬇️
src/AbsoluteCellRange.ts 86.97% <82.85%> (+0.14%) ⬆️
src/ArrayValue.ts 84.00% <100.00%> (ø)
src/Config.ts 98.66% <100.00%> (+0.01%) ⬆️
src/DependencyGraph/FormulaCellVertex.ts 86.51% <100.00%> (+0.20%) ⬆️
src/HyperFormula.ts 97.60% <100.00%> (+<0.01%) ⬆️

Copy link
Contributor

@wojciechczerniak wojciechczerniak left a comment

Choose a reason for hiding this comment

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

ConfigParamsNoDeprecated does not include gpu.

But I think we need different solution than introducing another type

@izulin
Copy link
Collaborator Author

izulin commented Oct 6, 2021

ConfigParamsNoDeprecated does not include gpu.

gpu is not yet removed in the develop, right?

@wojciechczerniak
Copy link
Contributor

ConfigParamsNoDeprecated does not include gpu.

gpu is not yet removed in the develop, right?

No, but it's deprecates just like binaryThreshold

hyperformula/src/Config.ts

Lines 692 to 696 in 0e321bf

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

@izulin
Copy link
Collaborator Author

izulin commented Oct 6, 2021

ConfigParamsNoDeprecated does not include gpu.

gpu is not yet removed in the develop, right?

No, but it's deprecates just like binaryThreshold

hyperformula/src/Config.ts

Lines 692 to 696 in 0e321bf

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

But those options are still actually used, they have effect, so when asking for getConfig() we have to output them. binaryThreshold had no effect so can be omited.

@wojciechczerniak
Copy link
Contributor

ConfigParamsNoDeprecated does not include gpu.

gpu is not yet removed in the develop, right?

No, but it's deprecates just like binaryThreshold

hyperformula/src/Config.ts

Lines 692 to 696 in 0e321bf

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

But those options are still actually used, they have effect, so when asking for getConfig() we have to output them. binaryThreshold had no effect so can be omited.

Sure. But if it's set anyway it should be there.

We just need the defaultConfig to omit it when used internally.

@izulin
Copy link
Collaborator Author

izulin commented Oct 7, 2021

ok, let's try something simpler

Copy link
Contributor

@wojciechczerniak wojciechczerniak left a comment

Choose a reason for hiding this comment

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

Simple and elegant solution 👍

@izulin izulin merged commit ae21079 into develop Oct 11, 2021
@izulin izulin deleted the pu/configwarnings branch October 11, 2021 13:23
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.

3 participants