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

Animator non nil #2981

Merged
merged 9 commits into from
Nov 24, 2017
Merged

Animator non nil #2981

merged 9 commits into from
Nov 24, 2017

Conversation

jjatie
Copy link
Collaborator

@jjatie jjatie commented Nov 13, 2017

Animator is now non-optional in Renderer types

@codecov-io
Copy link

codecov-io commented Nov 13, 2017

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Member

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
Copy link
Collaborator Author

jjatie commented Nov 18, 2017

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.

@@ -15,13 +15,13 @@ import CoreGraphics
@objc(ChartDataRendererBase)
open class DataRenderer: Renderer
{
@objc open var animator: Animator?
@objc open let animator: Animator
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@jjatie jjatie Nov 20, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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
Copy link
Member

liuxuan30 commented Nov 20, 2017

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

@jjatie
Copy link
Collaborator Author

jjatie commented Nov 20, 2017

Fixed

@liuxuan30
Copy link
Member

liuxuan30 commented Nov 22, 2017

@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
Copy link
Member

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

@liuxuan30 liuxuan30 merged commit 122013e into ChartsOrg:master Nov 24, 2017
@jjatie jjatie deleted the animator-non-nil branch November 24, 2017 13:33
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