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

Add Collection conformances to ChartDataSet types #3815

Merged
merged 3 commits into from
Jan 28, 2019

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

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


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

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

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

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

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

@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

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

sure. let's do this with another PR

@liuxuan30
Copy link
Member

@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

@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

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