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 changes to Animator #3005

Merged
merged 2 commits into from
Nov 17, 2017
Merged

Minor changes to Animator #3005

merged 2 commits into from
Nov 17, 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 #3005 into master will increase coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3005      +/-   ##
==========================================
+ Coverage   19.64%   19.68%   +0.04%     
==========================================
  Files         113      113              
  Lines       13792    13762      -30     
==========================================
  Hits         2709     2709              
+ Misses      11083    11053      -30
Impacted Files Coverage Δ
Source/Charts/Animation/Animator.swift 1.61% <0%> (+0.31%) ⬆️

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 a1811c7...ee337d7. Read the comment docs.

phaseY = Double(elapsed / duration)
}

phaseY = _easingY?(elapsed, duration) ?? elapsed / duration
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 using if let here instead of a one-liner here would be more readable

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 disagree on this one. ?? is a very common operator in Swift (it's all over the stdlib). Would you prefer something like

phaseY = _easingY?(elapsed, duration)
    ?? elapsed / duration

or

phaseY = _easingY?(elapsed, duration) ??
    elapsed / duration

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 also worth noting that on #2698 there were many cases of this being approved, including this line (thought it was written as phaseY = _easingY?(elapsed, duration) ?? Double(elapsed / duration)

Copy link
Collaborator

Choose a reason for hiding this comment

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

My problem is not with the ?? My problem is with the optional closure. I think It would be clearer to unwrap it first with if let before calling it. It is minor though. It’s up to you.

Copy link
Collaborator Author

@jjatie jjatie Nov 15, 2017

Choose a reason for hiding this comment

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

I would agree with you in a case like:

let x = myFuncWithNonEscaping? {
     // Logic
} ?? myDefaultValue

But in this case I really think this is okay.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I find it cluttered but it doesn’t really matter.

@pmairoldi pmairoldi merged commit 19e8b00 into ChartsOrg:master Nov 17, 2017
@jjatie jjatie deleted the animator-cleanup branch November 18, 2017 00:01
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