Animator non nil#2981
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
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. |
|
I can start flattening my commits. The framework is not designed to allow for |
| open class DataRenderer: Renderer | ||
| { | ||
| @objc open var animator: Animator? | ||
| @objc open let animator: Animator |
There was a problem hiding this comment.
did Xcode tell you to use let instead of var? I wonder if any chance people would modify renderer's animator later.
There was a problem hiding this comment.
I made it let. What is the use case of changing animator?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
oops, looks like there are dependency or orders. conflict when #3010 merged, but easy to fix. |
|
Fixed |
|
@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. |
|
I will take this then. seems many of these PRs there I could handle :) |
Animatoris now non-optional inRenderertypes