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

Minor refactoring of Formatter logic #2998

Merged
merged 4 commits into from
Dec 24, 2017
Merged

Minor refactoring of Formatter logic #2998

merged 4 commits into from
Dec 24, 2017

Conversation

jjatie
Copy link
Collaborator

@jjatie jjatie commented Nov 14, 2017

No description provided.

@codecov-io
Copy link

codecov-io commented Nov 14, 2017

Codecov Report

Merging #2998 into master will increase coverage by 1.31%.
The diff coverage is 15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2998      +/-   ##
==========================================
+ Coverage   19.45%   20.77%   +1.31%     
==========================================
  Files         113      113              
  Lines       16044    16448     +404     
  Branches      247      289      +42     
==========================================
+ Hits         3122     3417     +295     
- Misses      12884    12992     +108     
- Partials       38       39       +1
Impacted Files Coverage Δ
...ce/Charts/Formatters/IndexAxisValueFormatter.swift 0% <0%> (ø) ⬆️
.../Charts/Formatters/DefaultAxisValueFormatter.swift 54% <33.33%> (-2.61%) ⬇️
...urce/Charts/Formatters/DefaultValueFormatter.swift 52% <33.33%> (-2.72%) ⬇️
...ource/Charts/Formatters/DefaultFillFormatter.swift 4% <7.69%> (-1.67%) ⬇️
...ta/Implementations/Standard/LineChartDataSet.swift 24.34% <0%> (-0.35%) ⬇️
Source/Charts/Charts/PieChartView.swift 0% <0%> (ø) ⬆️
...a/Implementations/Standard/PieChartDataEntry.swift 0% <0%> (ø) ⬆️
...a/Implementations/Standard/BarChartDataEntry.swift 2.55% <0%> (+0.08%) ⬆️
Source/Charts/Charts/ChartViewBase.swift 27.21% <0%> (+5.86%) ⬆️
Source/Charts/Components/AxisBase.swift 51.8% <0%> (+6.47%) ⬆️
... and 2 more

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 159e0f7...3ab6706. Read the comment docs.

{
return formatter?.string(from: NSNumber(floatLiteral: value)) ?? ""
}
return block?(value, axis) ??
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if let would be more readable in this case.

{
return formatter?.string(from: NSNumber(floatLiteral: value)) ?? ""
}
return block?(value, entry, dataSetIndex, viewPortHandler) ??
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if let would be more readable in this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I flip-flopped on this one a lot. I'll change it now.

@jjatie
Copy link
Collaborator Author

jjatie commented Nov 15, 2017

Changes have been made

@jjatie
Copy link
Collaborator Author

jjatie commented Nov 23, 2017

@petester42 Any further feedback on this one or can it be merged?

@jjatie jjatie merged commit 8b18e76 into ChartsOrg:master Dec 24, 2017
@jjatie jjatie deleted the minor-formatter-logic branch December 24, 2017 16:39
@danielgindi
Copy link
Collaborator

Actually the naming of the commit / PR itself could have been better, as it does not change the logic - it's a refactoring :-)

@liuxuan30
Copy link
Member

and I already told him to try squash before merging and our idea for managing commits

@jjatie jjatie changed the title Minor changes to Formatter logic Minor refactoring to Formatter logic Dec 25, 2017
@jjatie jjatie changed the title Minor refactoring to Formatter logic Minor refactoring of Formatter logic Dec 25, 2017
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.

5 participants