Skip to content

Add Collection conformances to ChartDataSet types#3815

Merged
liuxuan30 merged 3 commits intomasterfrom
dataset-collection-conformance
Jan 28, 2019
Merged

Add Collection conformances to ChartDataSet types#3815
liuxuan30 merged 3 commits intomasterfrom
dataset-collection-conformance

Conversation

@jjatie
Copy link
Collaborator

@jjatie jjatie commented Jan 13, 2019

Goals ⚽

Allow more familiar API for collection types, the ability to implement more generic algorithms and simplify many existing algorithms by taking advantage of Collection conformances.

Implementation Details 🚧

ChartDataSet adopts conformances:
MutableCollection
RandomAccessCollection
RangeReplaceableCollection

@liuxuan30
Copy link
Member

liuxuan30 commented Jan 14, 2019

should we use 4.0 branch? I suppose there would be conflicts later.

liuxuan30
liuxuan30 reviewed Jan 14, 2019
View reviewed changes
liuxuan30
liuxuan30 reviewed Jan 22, 2019
View reviewed changes
Source/Charts/Data/Implementations/Standard/ChartDataSet.swift

return removed
guard let index = firstIndex(of: entry) else { return false }
_ = remove(at: index)
Copy link
Member

@liuxuan30 liuxuan30 Jan 22, 2019

Choose a reason for hiding this comment

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

I'm checking isIndirectValuesCall here, it seems in remove(at:), we didn't call isIndirectValuesCall = true and we delete it here. Is it a mistake?

Copy link
Collaborator Author

@jjatie jjatie Jan 22, 2019

Choose a reason for hiding this comment

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

This was on purpose. We always want to call notifiyDataSetChanged in our current implementation, so I go rid of it and let the property observer take car of it.

@liuxuan30
Copy link
Member

liuxuan30 commented Jan 22, 2019

BTW, should we use 4.0 branch? I suppose there would be conflicts later? I'm going to try merge a few PRs and release a new version. Not sure if we should include this one.

@codecov-io
Copy link

codecov-io commented Jan 22, 2019 •
edited
Loading

Codecov Report

Merging #3815 into master will decrease coverage by 0.03%.
The diff coverage is 15.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3815      +/-   ##
==========================================
- Coverage   32.67%   32.63%   -0.04%     
==========================================
  Files         114      114              
  Lines       10754    10809      +55     
==========================================
+ Hits         3514     3528      +14     
- Misses       7240     7281      +41
Impacted Files Coverage Δ
...s/Data/Implementations/Standard/ChartDataSet.swift 21.05% <15.38%> (-2.29%) ⬇️
Source/Charts/Charts/BarLineChartViewBase.swift 62.19% <0%> (-4.92%) ⬇️
Source/Charts/Charts/ChartViewBase.swift 26.49% <0%> (+3.32%) ⬆️
...Data/Implementations/Standard/ChartDataEntry.swift 60% <0%> (+3.33%) ⬆️

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 0749a2e...5e8c9b1. Read the comment docs.

@jjatie
Copy link
Collaborator Author

jjatie commented Jan 22, 2019

I think the deprecations should come in before Chart 4. There are already a ton of conflicts, so it doesn't matter to me

@liuxuan30 liuxuan30 merged commit 8d38438 into master Jan 28, 2019
@liuxuan30
Copy link
Member

liuxuan30 commented Feb 13, 2019 •
edited
Loading

@jjatie while I'm pushing pod trunk, got several warnings from pod:

Validating podspec
 -> Charts (3.2.2)
    - NOTE  | [Charts/Core] xcodebuild:  note: Using new build system
    - NOTE  | [Charts/Core] xcodebuild:  note: Planning build
    - NOTE  | [Charts/Core] xcodebuild:  note: Constructing build description
    - NOTE  | [Charts/Core] xcodebuild:  warning: Skipping code signing because the target does not have an Info.plist file. (in target 'App')
    - WARN  | [Charts/Core] xcodebuild:  Charts/Source/Charts/Data/Implementations/Standard/BarChartDataSet.swift:22:37: warning: 'values' is deprecated: Use `subscript(position:)` instead.
    - WARN  | [Charts/Core] xcodebuild:  Charts/Source/Charts/Data/Implementations/Standard/BarChartDataSet.swift:23:53: warning: 'values' is deprecated: Use `subscript(position:)` instead.
    - WARN  | [Charts/Core] xcodebuild:  Charts/Source/Charts/Data/Implementations/Standard/ChartDataSet.swift:29:9: warning: 'values' is deprecated: Use `subscript(position:)` instead.
    - WARN  | [Charts/Core] xcodebuild:  Charts/Source/Charts/Data/Implementations/Standard/ChartDataSet.swift:36:9: warning: 'values' is deprecated: Use `subscript(position:)` instead.
    - WARN  | [Charts/Core] xcodebuild:  Charts/Source/Charts/Data/Implementations/Standard/ChartDataSet.swift:43:14: warning: 'values' is deprecated: Use `subscript(position:)` instead.
    - WARN  | [Charts/Core] xcodebuild:  Charts/Source/Charts/Data/Implementations/Standard/ChartDataSet.swift:215:19: warning: 'values' is deprecated: Use `subscript(position:)` instead.
    - WARN  | [Charts/Core] xcodebuild:  Charts/Source/Charts/Data/Implementations/Standard/ChartDataSet.swift:216:20: warning: 'values' is deprecated: Use `subscript(position:)` instead.
    - WARN  | [Charts/Core] xcodebuild:  Charts/Source/Charts/Data/Implementations/Standard/ChartDataSet.swift:221:25: warning: 'values' is deprecated: Use `subscript(position:)` instead.
...
[!] The spec did not pass validation, due to 38 warnings (but you can use `--allow-warnings` to ignore them).

I'm using --allow-warnings for trunk push instead. We could start fixing those warnings next time

@jjatie
Copy link
Collaborator Author

jjatie commented Feb 13, 2019

Those are warnings intentionally generated by us. We won't be removing values but removing it's public exposure, so it's fine.

@jjatie jjatie deleted the dataset-collection-conformance branch February 13, 2019 02:18
@liuxuan30
Copy link
Member

liuxuan30 commented Feb 13, 2019

but if we don't have to remove values, we need find a way to get rid of the warnings

@jjatie
Copy link
Collaborator Author

jjatie commented Feb 13, 2019 

    @available(*, deprecated, message: "Use `subscript(position:)` instead.")
    @objc open var values: [ChartDataEntry]
        {
            get { return _values }
            set { _values = newValue }
        }
    internal var _values: [ChartDataEntry]
        {
        didSet
        {
            if isIndirectValuesCall {
                isIndirectValuesCall = false
                return
            }
            notifyDataSetChanged()
        }
    }

and then internally we only reference the internal var.

@liuxuan30
Copy link
Member

liuxuan30 commented Feb 13, 2019

sure. let's do this with another PR

@liuxuan30
Copy link
Member

liuxuan30 commented Feb 13, 2019

@jjatie I'm trying to fix the warnings, but I found there are demos using values like

    BarChartDataSet *set = nil;
    if (_chartView.data.dataSetCount > 0)
    {
        set = (BarChartDataSet *)_chartView.data.dataSets[0];
        set.values = yValues;
        [_chartView.data notifyDataChanged];
        [_chartView notifyDataSetChanged];
    }

what you suggest? btw, why do we want to deprecate values?

@liuxuan30
Copy link
Member

liuxuan30 commented Feb 18, 2019 •
edited
Loading

@jjatie can you get this notification? I didn't have a perfect idea yet, I'm thinking disable deprecate until we find a good way.
Either we add a new API or we don't encourage people to do so anymore. But I suppose. keeping values public is fine

@liuxuan30 liuxuan30 mentioned this pull request Feb 18, 2019
Merged
@liuxuan30 liuxuan30 mentioned this pull request Jan 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liuxuan30 liuxuan30 liuxuan30 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jjatie @liuxuan30 @codecov-io