Adopted `==` in favour of `isEqual(to:)` #2730

jjatie · 2017-08-18T07:26:19Z

Moved public func ==(_:_:) -> Bool into type extensions as static functions to properly adopt the Equatable protocol.
All instances of isEqual(to:) have been replaced with ==, but the isEqual(to:) methods remain for compatibility with objective-c

Moved `public func ==(_:_:) -> Bool` into type extensions as static functions to properly adopt the `Equatable` protocol. All instances of `isEqual(to:)` have been replaced with `==`, but the `isEqual(to:)` methods remain for compatibility with objective-c

codecov-io · 2017-08-18T07:35:06Z

Codecov Report

Merging #2730 into master will increase coverage by 0.63%.
The diff coverage is 42.39%.

@@            Coverage Diff             @@
##           master    #2730      +/-   ##
==========================================
+ Coverage   19.64%   20.27%   +0.63%     
==========================================
  Files         113      113              
  Lines       13792    13644     -148     
==========================================
+ Hits         2709     2767      +58     
+ Misses      11083    10877     -206

Impacted Files	Coverage Δ
Tests/Charts/ChartUtilsTests.swift	`100% <ø> (ø)`	⬆️
Source/Charts/Renderers/LineRadarRenderer.swift	`7.14% <ø> (ø)`	⬆️
...Data/Implementations/Standard/RadarChartData.swift	`0% <ø> (ø)`	⬆️
Source/Charts/Jobs/AnimatedZoomViewJob.swift	`0% <ø> (ø)`	⬆️
Source/Charts/Charts/CombinedChartView.swift	`0% <ø> (ø)`	⬆️
Source/Charts/Highlight/Range.swift	`0% <ø> (ø)`	⬆️
...rts/Renderers/Scatter/ChevronUpShapeRenderer.swift	`0% <ø> (ø)`	⬆️
...ource/Charts/Formatters/DefaultFillFormatter.swift	`5% <ø> (ø)`	⬆️
Source/Charts/Charts/BarChartView.swift	`32.6% <ø> (ø)`	⬆️
...ta/Implementations/Standard/ScatterChartData.swift	`0% <ø> (ø)`	⬆️
... and 85 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 a1811c7...c83e3ea. Read the comment docs.

liuxuan30 · 2017-08-21T02:04:06Z

Source/Charts/Data/Implementations/Standard/ChartDataEntry.swift

+
+        return lhs.isKind(of: type(of: rhs))
+            && (lhs.data?.isEqual(rhs.data) ?? true)
+            && lhs.y == rhs.y


It seems you replaced if fabs(lhs.x - rhs.x) > Double.ulpOfOne with lhs.x == rhs.x. Any support material? Want to be cautious about this change.

I agree. The Swift core says it implements == for FloatingPoint according to IEEE 754-2008. Unfortunately I can't quote this due to the nature of IEEE documents, but we should be fine using the built in == and not have to rely on ulpOfOne.

liuxuan30 · 2017-08-21T02:08:57Z

Source/Charts/Data/Implementations/Standard/ChartDataEntry.swift

+        }
+
+        return lhs.isKind(of: type(of: rhs))
+            && (lhs.data?.isEqual(rhs.data) ?? true)


I think we should be careful with this ?? true. what if lhs.data is nil when rhs.data is nil, or rhs.data is not nil?

I will fix it now.

On second thought, do we want to include the data property in the equality test at all? I can see an argument for either side, but I think equality can omit data. Testing identity (===) will compare data inherently.

I am not sure why data is included here. But @danielgindi is missing :(
Why can't we just keep the old conditions like

&& lhs.data !== rhs.data && !lhs.data!.isEqual(rhs.data)

ideally data will not be nil, if it's nil we will get the exception, but the new code will cover it up by returning false

!== shouldn't be used. Equality and identity are two different things. Also, my next step is to turn ChartDataEntry into a struct which means it's impossible to to test identity.

The new condition I just added tests equality with data properly. I think for now, we should just accept that data will be tested.

The final section of this API Doc explains the difference between equality and identity, and why identity should not be considered when implementing Equatable. There is an argument to be made for the first check (if lhs === rhs { return true }), because it allows a quick escape without having to check each parameter individually (though I still disagree with it. I'm curious to see the performance of that identity check vs simply checking the equality of the ivars).

In the example you gave with LineChartData and BarChartData, using === would return true on those entries and using == would also return true.

With data there are 5 possible scenarios

lhs, rhs 1. nil, nil 2. val1, nil 3. nil, val1 4. val1, val2 5. val1, val1

using lhs.data?.isEqual(rhs.data) ?? false will provide the correct result in cases 2, 3, 4, and 5. In order to account for case 1, we must also check to see if they are both nil.

To address your other point, there are a few areas where Charts has some issues (e.g. large data sets, performance with real time data, some API oddities). It is also clearly not a Swift project. I understand the language has evolved quite a bit since it's original release, but there are some key areas (e.g. why do ChartData and ChartDataSets not conform to the appropriate Collection and Equatable protocols? Why is ChartDataEntry [and ideally ChartData and ChartDataSets] a class and not a struct?) that could use enhancements to make it feel more like a Swift library.

I get the impression from the readme that this is supposed to be a community project. It's fine that the lead maintainer is away to be with their family. It's been a few months now without him though, so how long do we wait until we choose someone else to lead the project? Ideally more than one person so as to not fall in the same trap of having nobody qualified to make bigger decisions again? From reading responses to issues and PRs, it seems nobody currently reviewing them really has sufficient knowledge of Swift to make a decision on more fundamental changes to the framework (which will be required to handle larger datasets).

The frustrating part isn't that someone doesn't have time to look at a given PR, it's that even if they do nothing will happen. For example: this is a pretty simple PR that doesn't change the usage or performance of the framework, and is something that is required to make the framework "more Swifty" and (I can't say required) is a helpful step in making the framework be much more performant. With the exception of missing the case when both lhs.data and rhs.data are nil, there is no reason for it not to be accepted.

please allow me to find spare time to digest a little. You are giving too much information for me :) but that's good swift expert is helping us

for your second reply, yes it's bothering us (me) that we all have work to do and not having time to track the swift evolution. It could be lazy or just too many family errands. I am currently the only active person reviewing issues and PRs. But I am not swift expert and for those swift specific improvements, I need to find a time sit down and digest first. If it's small changes, I could merge for sure. But for very big changes, especially that will remove the support to objc usage, this gonna take time.

You are welcome on board to help us. You can try gitter or send emails to @danielgindi. We need to find out a solution for how to embrace swift better.

for the first point regards the code, sure we have if lhs === rhs { return true } already, but ChartDatEntry's data is AnyObject? first. Checking identify first will avoid checking equatable if they are not same class at first, right? If we don't use identity check, then we need to make sure equatable protocol works in any scenario

liuxuan30 · 2017-08-21T03:03:03Z

Source/Charts/Data/Implementations/Standard/ChartDataEntryBase.swift

+            return true
+        }
+        return lhs.isKind(of: type(of: rhs))
+            && (lhs.data?.isEqual(rhs.data) ?? true)


after we come to agreement, this needs change too

jjatie · 2017-08-22T05:34:03Z

The requested changes have been made...

pmairoldi · 2017-09-07T04:11:10Z

This change looks ok for me but i'd like to add this after swift 4 support is merge. Should be soon!

liuxuan30 · 2017-09-12T00:59:39Z

@petester42
shall we remove

if lhs.data !== rhs.data

here? My concern is in extreme cases different data class may be passed through, and they have the same entries... Like a BarData and LineData hold the same entries

pmairoldi · 2017-09-12T01:12:42Z

That wouldn't be true though right? lhs.data and rhs.data would have to be the same instance of an object.

liuxuan30 · 2017-09-12T01:23:34Z

My bad, lhs.data refers to open var data: AnyObject?, not ChartData. In this case, I think it's not safe to remove the identity check? Old code has if lhs.data !== rhs.data check in ChartDataEntryBase but I don't obviously see the new code check the class type

jjatie · 2017-09-17T15:11:17Z

@petester42 What are your thoughts on removing data from the equality check altogether? My reasoning being that the logical equality of two data points should only take into consideration the x and y values.

pmairoldi · 2017-09-17T18:44:18Z

Depends on what the intent of the equality should be. The equality to me should be that the objects are the same. If there is need to compare only part of the object like x and y values then some compare function should exists like a.hasSamePoint(b).

jjatie · 2017-10-19T16:16:50Z

Can this be merged now that we've moved to swift 4?

pmairoldi · 2017-10-21T03:06:49Z

Source/Charts/Data/Implementations/Standard/ChartDataEntry.swift

-
-    if !lhs.isKind(of: type(of: rhs))
+// MARK: Equatable
+extension ChartDataEntry/*: Equatable*/ {


Why is equatable commented out?

Because NSObject adopts Equatable and you can't declare conformance twice. When Obj-C support is dropped, then we will need to uncomment it.

pmairoldi · 2017-10-21T03:07:18Z

Source/Charts/Data/Implementations/Standard/ChartDataEntryBase.swift

-{
-    if lhs === rhs
+// MARK: Equatable
+extension ChartDataEntryBase/*: Equatable*/ {


Why is equatable commented out?

Because NSObject adopts Equatable and you can't declare conformance twice. When Obj-C support is dropped, then we will need to uncomment it.

pmairoldi · 2017-10-21T03:07:27Z

Source/Charts/Highlight/Highlight.swift

-{
-    if lhs === rhs
+// MARK: Equatable
+extension Highlight /*: Equatable*/ {


Why is equatable commented out?

Because NSObject adopts Equatable and you can't declare conformance twice. When Obj-C support is dropped, then we will need to uncomment it.

pmairoldi · 2017-10-21T03:08:37Z

@jjatie I still have issues with this. If tests could be added for this change that would be great since it would be fairly easy to add.

jjatie · 2017-11-06T19:07:04Z

@petester42 I'm not sure what tests you want exactly. I'll write one for the equality/inequality of the couple types that are now using ==. Are there any more you'd like?

…quatable

According to [Apple](https://developer.apple.com/library/content/documentation/Swift/Conceptual/BuildingCocoaApps/InteractingWithObjective-CAPIs.html#//apple_ref/doc/uid/TP40014216-CH4-ID35) >Swift provides default implementations of the == and === operators and adopts the Equatable protocol for objects that derive from the NSObject class. The default implementation of the == operator invokes the isEqual: method, and the default implementation of the === operator checks pointer equality. You should not override the equality or identity operators for types imported from Objective-C.

jjatie · 2017-11-14T04:20:52Z

@petester42 I have reverted back to isEqual declarations, but using == to follow Swift convention because I found this:

According to Apple

Swift provides default implementations of the == and === operators and adopts the Equatable protocol for objects that derive from the NSObject class. The default implementation of the == operator invokes the isEqual: method, and the default implementation of the === operator checks pointer equality. You should not override the equality or identity operators for types imported from Objective-C.

It appears I made a horrible merge, bringing back things from before Swift 4 was adopted, so I will submit a new PR (#3002)

liuxuan30 · 2017-11-18T02:35:48Z

can this be closed if you replaced with a new one?

Resubmit of #2730

liuxuan30 reviewed Aug 21, 2017

View reviewed changes

data is now properly tested for equality in ChartDataEntry

ed74674

liuxuan30 reviewed Aug 21, 2017

View reviewed changes

ChartDataEntryBase also test equality properly now.

ea19ae4

pmairoldi reviewed Oct 21, 2017

View reviewed changes

jjatie added 3 commits November 13, 2017 18:21

Added Equatable tests

04843e1

Merge branch 'master' of https://github.com/danielgindi/Charts into e…

09a53a7

…quatable

jjatie mentioned this pull request Nov 14, 2017

Resubmit of #2730 #3002

Merged

jjatie closed this Nov 20, 2017

jjatie added a commit that referenced this pull request Dec 24, 2017

Merge pull request #3002 from jjatie/equatable-fixed

428843f

Resubmit of #2730

jjatie deleted the equatable branch January 6, 2018 14:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopted `==` in favour of `isEqual(to:)` #2730

Adopted `==` in favour of `isEqual(to:)` #2730

jjatie commented Aug 18, 2017

codecov-io commented Aug 18, 2017 •

edited

Loading

liuxuan30 Aug 21, 2017 •

edited

Loading

jjatie Aug 21, 2017

liuxuan30 Aug 21, 2017 •

edited

Loading

jjatie Aug 21, 2017

jjatie Aug 21, 2017

liuxuan30 Aug 21, 2017

jjatie Aug 21, 2017 •

edited

Loading

jjatie Aug 23, 2017 •

edited

Loading

jjatie Aug 23, 2017

liuxuan30 Aug 23, 2017 •

edited

Loading

liuxuan30 Aug 23, 2017

liuxuan30 Aug 23, 2017 •

edited

Loading

liuxuan30 Aug 21, 2017

jjatie commented Aug 22, 2017

pmairoldi commented Sep 7, 2017 •

edited

Loading

liuxuan30 commented Sep 12, 2017

pmairoldi commented Sep 12, 2017

liuxuan30 commented Sep 12, 2017 •

edited

Loading

jjatie commented Sep 17, 2017 •

edited

Loading

pmairoldi commented Sep 17, 2017

jjatie commented Oct 19, 2017

pmairoldi Oct 21, 2017

jjatie Nov 14, 2017

pmairoldi Oct 21, 2017

jjatie Nov 14, 2017

pmairoldi Oct 21, 2017

jjatie Nov 14, 2017

pmairoldi commented Oct 21, 2017

jjatie commented Nov 6, 2017

jjatie commented Nov 14, 2017 •

edited

Loading

liuxuan30 commented Nov 18, 2017

Adopted == in favour of isEqual(to:) #2730

Adopted == in favour of isEqual(to:) #2730

Conversation

jjatie commented Aug 18, 2017

codecov-io commented Aug 18, 2017 • edited Loading

Codecov Report

liuxuan30 Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuxuan30 Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjatie Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

jjatie Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuxuan30 Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuxuan30 Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjatie commented Aug 22, 2017

pmairoldi commented Sep 7, 2017 • edited Loading

liuxuan30 commented Sep 12, 2017

pmairoldi commented Sep 12, 2017

liuxuan30 commented Sep 12, 2017 • edited Loading

jjatie commented Sep 17, 2017 • edited Loading

pmairoldi commented Sep 17, 2017

jjatie commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmairoldi commented Oct 21, 2017

jjatie commented Nov 6, 2017

jjatie commented Nov 14, 2017 • edited Loading

liuxuan30 commented Nov 18, 2017

Adopted `==` in favour of `isEqual(to:)` #2730

Adopted `==` in favour of `isEqual(to:)` #2730

codecov-io commented Aug 18, 2017 •

edited

Loading

liuxuan30 Aug 21, 2017 •

edited

Loading

liuxuan30 Aug 21, 2017 •

edited

Loading

jjatie Aug 21, 2017 •

edited

Loading

jjatie Aug 23, 2017 •

edited

Loading

liuxuan30 Aug 23, 2017 •

edited

Loading

liuxuan30 Aug 23, 2017 •

edited

Loading

pmairoldi commented Sep 7, 2017 •

edited

Loading

liuxuan30 commented Sep 12, 2017 •

edited

Loading

jjatie commented Sep 17, 2017 •

edited

Loading

jjatie commented Nov 14, 2017 •

edited

Loading