Add missing properties to copy(with:) methods

[dstranz]

Issue Link 🔗

#3578

Goals ⚽

• Add missing properties to copy(with:) methods

Implementation Details 🚧

Add any missing property to copy(with:) methods

Testing Details 🔍

I didn't add any tests cases, but if you think it will be valuable let me know.

#hacktoberfest #coinpaprika

[codecov-io]

Codecov Report

Merging #3715 into master will increase coverage by 1.66%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3715      +/-   ##
==========================================
+ Coverage   31.01%   32.67%   +1.66%     
==========================================
  Files         114      114              
  Lines       10482    10754     +272     
==========================================
+ Hits         3251     3514     +263     
- Misses       7231     7240       +9

Impacted Files	Coverage Δ
...Data/Implementations/Standard/ChartDataEntry.swift	56.66% <ø> (ø)	⬆️
...s/Data/Implementations/Standard/ChartDataSet.swift	23.33% <ø> (ø)	⬆️
...ata/Implementations/Standard/BarChartDataSet.swift	62.68% <0%> (-1.93%)	⬇️
...a/Implementations/Standard/BarChartDataEntry.swift	1.96% <0%> (-0.02%)	⬇️
...ata/Implementations/Standard/PieChartDataSet.swift	28.57% <0%> (-18.49%)	⬇️
...ta/Implementations/Standard/LineChartDataSet.swift	30% <0%> (-2.44%)	⬇️
...mplementations/Standard/CandleChartDataEntry.swift	0% <0%> (ø)	⬆️
...Charts/Data/Implementations/ChartBaseDataSet.swift	14.4% <0%> (-1.68%)	⬇️
...Implementations/Standard/ScatterChartDataSet.swift	0% <0%> (ø)	⬆️
.../Implementations/Standard/BubbleChartDataSet.swift	0% <0%> (ø)	⬆️
... and 11 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 3b07904...fae3be7. Read the comment docs.

[liuxuan30]

thanks for this PR. I wonder if this is a complete list for now?

btw, I wonder if there is better ideas/solutions, because people would add new properties without remembering to add in copy(). Either could we tell whether we must add in copy(), or any smart way to auto add, or just by reviewers have to keep it in mind

@jjatie any thoughts

[jjatie]

Really, the only way to keep on top of this is to do it by hand. One solution would be to implement the data model as Swift structs, but only expose them through classes. We get copy semantics and only have to add one property to the copy override i.e:

internal struct DataEntry: Equatable {
    let x: Int
    let y: Int
}

@objc
public class ChartDataEntry: NSObject {

    internal private(set) var entry: DataEntry

    var x: Int {
        get { return entry.x }
        set(x) { entry.x = x }
    }
    
    var y: Int {
        get { return entry.y }
        set(y) { entry.y = y }
    }

    func copy(with zone: NSZone? = nil) -> Any {
        let copy = type(of: self).init()
        copy.entry = entry
    }
}

It might be possible too, to create a protocol for this i.e.

protocol DataEntryProtocol {
    var x: { get set }
    var y: { get set }
}

extension DataEntry: DataEntryProtocol { }
extension ChartDataEntry: DataEntryProtocol { }

Then we could update our API to accept the protocol instead of the concrete type. This would allow us to start developing a nicer Swift API, and move objc compatibility into its own library for Charts 4. Charts 5 could drop objc compatibility altogether, and then we can remove the protocol from the library too (possibly).

Obviously this is all outside the scope of this PR, but we should start thinking about way to separate Obj-C functionality from the core lib.

[jjatie]

Wow, there is a glaring error in Charts. We aren't actually conforming to NSCopying as the base method we use func copyWithZone(_:) -> AnyObject is not the method the protocol requires.
Please replace this with func copy(with zone: NSZone? = nil) -> Any everywhere in the library it is needed.

[dstranz]

@jjatie I've just added NSCopying conformance to base classes to have compiler validation & changed method names.

[liuxuan30]

I can't believe how func copyWithZone(_:) -> AnyObject is added, lol