[#2999]

[jjatie]

ChartDataSet adopts conformances:
MutableCollection
RandomAccessCollection
RangeReplaceableCollection

[codecov-io]

Codecov Report

Merging #3018 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3018      +/-   ##
==========================================
- Coverage   19.72%   19.66%   -0.07%     
==========================================
  Files         113      113              
  Lines       13701    13743      +42     
==========================================
  Hits         2702     2702              
- Misses      10999    11041      +42

Impacted Files	Coverage Δ
...s/Data/Implementations/Standard/ChartDataSet.swift	29.47% <0%> (-4.08%)	⬇️

---

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 19e8b00...a86efd4. Read the comment docs.

[pmairoldi]

I think implementing these on the concrete implementation is not the right approach. If we can we should use protocol extensions since we want all implementations of datasets to get this functionality.

[jjatie]

I entirely agree, however protocol extensions are a Swift only feature and therefor cannot be implemented on an @objc protocol. Either we get rid of objective-c, get rid of the DataSetProtocols or we accept that this is the best we can do right now.

[pmairoldi]

If we can't do it right then I need a good reason to do it at all. Are we gonna rewrite the internal logic to use map, filter and such or it is just for the consumers of the library. I don't see much value for the consumers since they probably already do those operations to their data before creating their dataset. What use cases can you think of for needing this?

[jjatie]

That's reasonable.

Being able to take advantage of the collection conformance in the framework itself will greatly reduce the amount of code there is to maintain, unfortunately we are unable to do that in this implementation.

For consumers of the framework, the benefit they get from this is not map, filter, reduce, but to be able to access data in a more standard way if they need to, using subscripting, index(where:), etc. That's the only justification I have for this implementation. Any future improvements to how this implemented will not change the current functionality. Because of this I think it would make sense to
allow this change and also add it to ChartsRealm, then in the future (likely when we drop Obj-C support) we can provide a better implementation that we can take advantage of internally.

I see your argument though, and am fine if you choose to just close the PR.

[liuxuan30]

I feel dropping ObjC support is not gonna happen in near future as long as apple/people still using it.

[pmairoldi]

Ya I’m leaning towards closing this PR. The only gain would be subscript access and we can just add that ourselves.