Animator non nil

[jjatie]

Animator is now non-optional in Renderer types

[codecov-io]

Codecov Report

Merging #2981 into master will increase coverage by <.01%.
The diff coverage is 19.23%.

@@            Coverage Diff             @@
##           master    #2981      +/-   ##
==========================================
+ Coverage   19.76%   19.76%   +<.01%     
==========================================
  Files         113      113              
  Lines       13673    13630      -43     
==========================================
- Hits         2702     2694       -8     
+ Misses      10971    10936      -35

Impacted Files	Coverage Δ
Source/Charts/Renderers/LineRadarRenderer.swift	7.14% <ø> (ø)	⬆️
...ource/Charts/Renderers/CombinedChartRenderer.swift	0% <ø> (ø)	⬆️
...rts/Renderers/LineScatterCandleRadarRenderer.swift	9.09% <ø> (ø)	⬆️
Source/Charts/Renderers/ScatterChartRenderer.swift	0% <0%> (ø)	⬆️
...ce/Charts/Renderers/CandleStickChartRenderer.swift	0% <0%> (ø)	⬆️
...Renderers/BarLineScatterCandleBubbleRenderer.swift	68% <0%> (ø)	⬆️
Source/Charts/Renderers/RadarChartRenderer.swift	0% <0%> (ø)	⬆️
.../Charts/Renderers/HorizontalBarChartRenderer.swift	0% <0%> (ø)	⬆️
Source/Charts/Renderers/BubbleChartRenderer.swift	0% <0%> (ø)	⬆️
Source/Charts/Renderers/PieChartRenderer.swift	0% <0%> (ø)	⬆️
... and 3 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 3b6eda1...326caac. Read the comment docs.

[liuxuan30]

why non nil?

BTW, I suggest merge small commits into one with more meaningful message, like put this into single one commit with a message 'Animator non nil'. I saw you have multiple 'Merging with upstream/remote', kind of unneccessary.

I can use 'Squash and merge' for this PR anyway, so next time maybe.

[jjatie]

I can start flattening my commits.

The framework is not designed to allow for animator == nil as made clear by the fact I was able to simply remove the guard-else { return } for every area animator is used. This should be reflected in the property declaration and the initializers.

[liuxuan30]

did Xcode tell you to use let instead of var? I wonder if any chance people would modify renderer's animator later.

[jjatie]

I made it let. What is the use case of changing animator?

[liuxuan30]

Not sure, just seeing it used to be var, though it was optional and some advanced user maybe? Using let will be more strict.
If we use var, will Xocde complain it is never re-assigned?

[jjatie]

It's an instance variable, so it's impossible for Xcode to know if it is never re-assigned.

It is more strict, but from what I can tell there is no consideration for having multiple animators. We have one class for it, and no protocols to define an interface. Overriding any functions in Animator, without diving into the framework to know exactly what needs to be there and what can be changed, will cause problems.

[liuxuan30]

oops, looks like there are dependency or orders. conflict when #3010 merged, but easy to fix.

[jjatie]

Fixed

[liuxuan30]

@petester42 do you want to take a look at this and #2982? I checked it is legit to allow animator and renderer non nil, as the callers passes an object already.

[liuxuan30]

I will take this then. seems many of these PRs there I could handle :)