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

Fix for #4274 string comparison issue in ChartData::getDataSetByLabel #4275

Merged
merged 1 commit into from
Jan 24, 2020

Conversation

PeterKaminski09
Copy link
Contributor

Fixes an issue with string comparison in ChartData for finding a dataset by its label (Fixes #4274)

ChartData::getDataSetByLabel has a bug where the string comparison is checking the same string against itself due to the functions parameter having the same name as a variable within the closures scope. This change renames the variable to fix the issue.

Issue Link 🔗

#4274

Goals ⚽

  • Fixes the bug
  • Adds some unit tests to prevent the relevant issue from regressing

Implementation Details 🚧

  • No major changes here, just a quick fix

Testing Details 🔍

  • Added some tests that check case sensitivity
  • Added some tests that ignore case
  • Added some tests that check for the expected nil case

…set by its label (Fixes ChartsOrg#4274)

ChartData::getDataSetByLabel has a bug where the string comparison is checking the same string against itself due to the functions parameter having the same name as a variable within the closures scope. This change renames the variable to fix the issue.
@codecov-io
Copy link

Codecov Report

Merging #4275 into master will increase coverage by 0.31%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4275      +/-   ##
==========================================
+ Coverage    41.5%   41.82%   +0.31%     
==========================================
  Files         123      124       +1     
  Lines       14069    14103      +34     
==========================================
+ Hits         5840     5899      +59     
+ Misses       8229     8204      -25
Impacted Files Coverage Δ
ChartDataTests.swift 100% <100%> (ø)
...arts/Data/Implementations/Standard/ChartData.swift 58.84% <100%> (+7.08%) ⬆️
...ta/Implementations/Standard/ScatterChartData.swift 25% <0%> (+25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bbec78...66d50a8. Read the comment docs.

@danielgindi
Copy link
Collaborator

What a funny bug. Thank you!

@danielgindi danielgindi merged commit 642d514 into ChartsOrg:master Jan 24, 2020
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.

ChartData::getDataSetByLabel always return the first IChartDataSet when ignorecase = true
3 participants