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

Renderer protocol #2991

Closed
wants to merge 26 commits into from
Closed

Renderer protocol #2991

wants to merge 26 commits into from

Conversation

jjatie
Copy link
Collaborator

@jjatie jjatie commented Nov 13, 2017

Renderer and DataRenderer are now protocols instead of classes.

@jjatie jjatie mentioned this pull request Nov 13, 2017
@jjatie
Copy link
Collaborator Author

jjatie commented Nov 13, 2017

@liuxuan30 @petester42 @danielgindi There are changes here that pave the way to much larger changes to the framework which allow better type safety and less reliance on optionals. This is immediately followed by #2990. Once these two are approved, there will be more changes that can be quickly and easily made to greater improve the type safety and the use of optionals across the entire framework.

@codecov-io
Copy link

codecov-io commented Nov 13, 2017

Codecov Report

❗ No coverage uploaded for pull request base (4.0.0@df62114). Click here to learn what that means.
The diff coverage is 18.91%.

Impacted file tree graph

@@           Coverage Diff            @@
##             4.0.0    #2991   +/-   ##
========================================
  Coverage         ?   20.58%           
========================================
  Files            ?      112           
  Lines            ?    15431           
  Branches         ?      256           
========================================
  Hits             ?     3176           
  Misses           ?    12218           
  Partials         ?       37
Impacted Files Coverage Δ
Source/Charts/Utils/ViewPortHandler.swift 28.18% <ø> (ø)
...ource/Charts/Renderers/CombinedChartRenderer.swift 0% <0%> (ø)
Source/Charts/Renderers/BubbleChartRenderer.swift 0% <0%> (ø)
Source/Charts/Renderers/PieChartRenderer.swift 0% <0%> (ø)
...ts/Renderers/YAxisRendererHorizontalBarChart.swift 0% <0%> (ø)
Source/Charts/Renderers/LegendRenderer.swift 41.52% <100%> (ø)
Source/Charts/Renderers/AxisRendererBase.swift 58.18% <100%> (ø)
...Renderers/BarLineScatterCandleBubbleRenderer.swift 46% <26.31%> (ø)

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 df62114...76dc5b9. Read the comment docs.

@jjatie
Copy link
Collaborator Author

jjatie commented Nov 20, 2017

This includes #2980

@jjatie
Copy link
Collaborator Author

jjatie commented Nov 24, 2017

@liuxuan30 After #2982 I think this one should be done next

@liuxuan30
Copy link
Member

oops just read your comment.
I would suggest you create an issue to track your PRs, like which are for 4.0 branch and which can be merged into master directly. I feel dizzy switching around the PRs not sure which to start with. Some "minor" changes have hundreds of diffs, which are not minor to review.

I would first start with diffs that are just two digits and later with the big ones.

@liuxuan30 liuxuan30 self-requested a review December 9, 2017 03:32
@jjatie
Copy link
Collaborator Author

jjatie commented Dec 9, 2017

I'll just change the destination branch on the PR to 4.0.0.

Also, I will start making more PRs into 4.0.0. I think some of my changes are big enough to warrant 4.0.0 even if they're not breaking.

@jjatie
Copy link
Collaborator Author

jjatie commented Dec 9, 2017

In the future I will create Issues and name the PRs after them.

@jjatie jjatie changed the base branch from master to 4.0.0 December 9, 2017 19:33
@jjatie jjatie added this to the 4.0.0 milestone Dec 24, 2017
@jjatie jjatie changed the base branch from 4.0.0 to master December 24, 2017 18:57
@jjatie jjatie changed the base branch from master to 4.0.0 December 24, 2017 18:58
@jjatie
Copy link
Collaborator Author

jjatie commented Dec 26, 2017

Continued in #3136

@jjatie jjatie closed this Dec 26, 2017
@jjatie jjatie deleted the renderer_protocol branch December 26, 2017 04:14
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