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

Replaced ChartUtils.Math in favour of an extension on FloatingPoint #2993

Merged
merged 8 commits into from
Dec 11, 2017
Merged

Replaced ChartUtils.Math in favour of an extension on FloatingPoint #2993

merged 8 commits into from
Dec 11, 2017

Conversation

jjatie
Copy link
Collaborator

@jjatie jjatie commented Nov 13, 2017

Increases readability, and in many cases removes a set of parentheses to ensure order of operations. Also can be applied to any FloatingPoint type, not just Double and CGFloat

Increases readability, and in many cases removes a set of parentheses to ensure order of operations.
@codecov-io
Copy link

codecov-io commented Nov 14, 2017

Codecov Report

Merging #2993 into master will increase coverage by 0.04%.
The diff coverage is 8.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2993      +/-   ##
==========================================
+ Coverage   19.41%   19.45%   +0.04%     
==========================================
  Files         113      113              
  Lines       16064    16044      -20     
  Branches      247      247              
==========================================
+ Hits         3119     3122       +3     
+ Misses      12907    12884      -23     
  Partials       38       38
Impacted Files Coverage Δ
Source/Charts/Renderers/PieChartRenderer.swift 0% <0%> (ø) ⬆️
Source/Charts/Utils/Fill.swift 0% <0%> (ø) ⬆️
Source/Charts/Charts/RadarChartView.swift 0% <0%> (ø) ⬆️
...rce/Charts/Renderers/XAxisRendererRadarChart.swift 0% <0%> (ø) ⬆️
...ts/Renderers/XAxisRendererHorizontalBarChart.swift 0% <0%> (ø) ⬆️
Source/Charts/Charts/PieChartView.swift 0% <0%> (ø) ⬆️
Source/Charts/Charts/PieRadarChartViewBase.swift 0% <0%> (ø) ⬆️
Source/Charts/Renderers/XAxisRenderer.swift 58.25% <100%> (ø) ⬆️
Source/Charts/Utils/ChartUtils.swift 38.14% <30.76%> (+1.11%) ⬆️
Source/Charts/Jobs/AnimatedMoveViewJob.swift 0% <0%> (ø) ⬆️
... and 1 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 2ff5cb5...3aba32d. Read the comment docs.

@jjatie
Copy link
Collaborator Author

jjatie commented Nov 24, 2017

@liuxuan30 @petester42 This is a quick and easy one.

@liuxuan30 liuxuan30 self-requested a review December 5, 2017 01:56
/// NOTE: Value must be in degrees
var normalizedAngle: Self {
let angle = truncatingRemainder(dividingBy: 360)
return (sign == .minus) ? angle + 360 : angle
Copy link
Member

Choose a reason for hiding this comment

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

the order is flipped. I need to refresh my mind first to see if they are equivalent. Or you already did that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They’re equivalent.

Copy link
Member

Choose a reason for hiding this comment

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

made some stupid equations :P

liuxuan30
liuxuan30 previously approved these changes Dec 8, 2017
/// NOTE: Value must be in degrees
var normalizedAngle: Self {
let angle = truncatingRemainder(dividingBy: 360)
return (sign == .minus) ? angle + 360 : angle
Copy link
Member

Choose a reason for hiding this comment

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

made some stupid equations :P

@@ -16,9 +16,26 @@ import CoreGraphics
import UIKit
#endif

extension FloatingPoint {
var deg2rad: Self {
Copy link
Member

Choose a reason for hiding this comment

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

Generally it's fine. But do you think we should follow the Camel-Case naming? like deg2Rad

Copy link
Collaborator Author

@jjatie jjatie Dec 8, 2017

Choose a reason for hiding this comment

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

Up to you. I tried deg2Rad and deg2RAD and they both look silly to me. I also considered inRadians and inDegrees

Copy link
Member

Choose a reason for hiding this comment

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

OK. How about we keep old FDEG2RAD to keep consitent?

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'll drop the F since it's implied as it's an extension on FloatingPoint

@liuxuan30
Copy link
Member

This is the one of the change I like most, indeed make it more swift and simple

@liuxuan30 liuxuan30 merged commit 159e0f7 into ChartsOrg:master Dec 11, 2017
@jjatie jjatie deleted the deg-rad branch December 11, 2017 01:57
FreddyZeng added a commit to FreddyZeng/Charts that referenced this pull request Jan 2, 2018
* 'master' of https://github.com/danielgindi/Charts: (23 commits)
  Update ViewPortHandler.swift (ChartsOrg#3143)
  add option to build demo projects unit tests on iOS (ChartsOrg#3121)
  Replaced relevant `ChartUtils` methods with `Double` extensions (ChartsOrg#2994)
  Update 4.0.0 with master (ChartsOrg#3135)
  Removed redundant ivars in BarLineChartViewBase (ChartsOrg#3043)
  fix ChartsOrg#1830. credit from ChartsOrg#2049 (ChartsOrg#2874)
  Makes ChartsDemo compiling again (ChartsOrg#3117)
  Fixed using wrong axis (Issue ChartsOrg#2257)
  Removed methods and properties deprecated in 1.0 (ChartsOrg#2996)
  for ChartsOrg#3061 revert animationUpdate() and animationEnd() not trigger crash if subclass does nothing
  The backing var is not necessary. (ChartsOrg#3000)
  Replaced `ChartUtils.Math` in favour of an extension on `FloatingPoint` (ChartsOrg#2993)
  Minor logic cleanup (ChartsOrg#3041)
  Fix a bug may cause infinite loop. (ChartsOrg#3073)
  for ChartsOrg#2745. chart should be weak.
  fileprivate -> private (ChartsOrg#3042)
  Removed `isKind(of:)`
  Removed @objc from internal properties
  Fixes for PR
  Made use of `==` where appropriate to simplify logic
  ...

# Conflicts:
#	Source/Charts/Data/Implementations/Standard/LineChartDataSet.swift
#	Source/Charts/Renderers/BarChartRenderer.swift
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