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

Axis renderer protocol #2990

Closed
wants to merge 36 commits into from
Closed

Axis renderer protocol #2990

wants to merge 36 commits into from

Conversation

jjatie
Copy link
Collaborator

@jjatie jjatie commented Nov 13, 2017

Slight more code, but allows for better type safety and removes a lot of unnecessary type conversion. It's a "Swift-ier" way.

Note that this builds off the changes made in #2991

@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 PR is preceded by #2991. 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 in 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 51.13%.

Impacted file tree graph

@@           Coverage Diff            @@
##             4.0.0    #2990   +/-   ##
========================================
  Coverage         ?   20.84%           
========================================
  Files            ?      111           
  Lines            ?    15454           
  Branches         ?      250           
========================================
  Hits             ?     3222           
  Misses           ?    12192           
  Partials         ?       40
Impacted Files Coverage Δ
Source/Charts/Utils/ViewPortHandler.swift 28.18% <ø> (ø)
Source/Charts/Renderers/BubbleChartRenderer.swift 0% <0%> (ø)
...ts/Renderers/YAxisRendererHorizontalBarChart.swift 0% <0%> (ø)
...rce/Charts/Renderers/YAxisRendererRadarChart.swift 0% <0%> (ø)
Source/Charts/Renderers/PieChartRenderer.swift 0% <0%> (ø)
...ource/Charts/Renderers/CombinedChartRenderer.swift 0% <0%> (ø)
...ts/Renderers/XAxisRendererHorizontalBarChart.swift 0% <0%> (ø)
...rce/Charts/Renderers/XAxisRendererRadarChart.swift 0% <0%> (ø)
Source/Charts/Renderers/LegendRenderer.swift 41.52% <100%> (ø)
...Renderers/BarLineScatterCandleBubbleRenderer.swift 46% <26.31%> (ø)
... and 2 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 df62114...41d5f10. Read the comment docs.

@liuxuan30
Copy link
Member

some of your PRs have conflicts. Any idea?
Also, thank you so much for improing, you are contributing a lot. But kind of stressful for so many changes in a short time. I guess we need a discussion first with @danielgindi and @petester42 ?

@jjatie
Copy link
Collaborator Author

jjatie commented Nov 23, 2017

The PRs have conflicts because others are being accepted and merged. I'll do a better job of keeping them up to date. Once #2980 is accepted and merged, #2991 and this one will be easier to review.

@liuxuan30
Copy link
Member

liuxuan30 commented Dec 9, 2017

feel PRs get a little messy.

I just found out there is 4.0 branch for breaking changes. do you have the priv to add labels for your PR to tag as 4.0? I can help review merge minor changes. But just get lost when finding some PRs that looks like for 4.0 branch.

What's the proposal for 4.0 branch? @jjatie do you switch all changes to 4.0 branch, and close existing one that's to master?

@jjatie jjatie changed the base branch from master to 4.0.0 December 9, 2017 19:34
@danielgindi
Copy link
Collaborator

Thanks!

This looks like a refactoring-only PR, but it includes 44 commits.
This should be squashed before merging... :-)

@pmairoldi
Copy link
Collaborator

It can be squashed when clicking the merge button.

@liuxuan30
Copy link
Member

liuxuan30 commented Dec 24, 2017

yep, with "Squash and merge" option (one of of the drill down menu lists), it can be done all in once.
@danielgindi have you added @jjatie in :)

@jjatie
Copy link
Collaborator Author

jjatie commented Dec 24, 2017

The reason there's so many commits is because it's PRd into 4.0.0 which isn't up to date with master. It also includes the changes made in #2991 as noted at the top.

@danielgindi
Copy link
Collaborator

I've added @jjatie :-) Welcome aboard!
@liuxuan30 have told me that you were trying to get my attention, but the problem is that there are so many Github notifications from issues and PRs, that I can't read all of them and then I miss some important ones.

@jjatie
Copy link
Collaborator Author

jjatie commented Dec 24, 2017

Thanks @danielgindi

@jjatie
Copy link
Collaborator Author

jjatie commented Dec 24, 2017

Given this seems to be approved, I will assume #2991 is approved as well. I'll PR master into 4.0.0 and start merging in PRs that have been approved for 4.0.0

@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 19:11
@jjatie jjatie changed the base branch from master to 4.0.0 December 24, 2017 19:12
@liuxuan30
Copy link
Member

@jjatie what's your proposal on reviewing 4.0 branch PRs? I asked in #3130, but seems not discussed.

@jjatie
Copy link
Collaborator Author

jjatie commented Dec 25, 2017

I'd say lets keep it the same as for the master.

Now that 4.0 and master will be getting different changes submitted to them, I will do my best to get the bug fixes from master into 4.0 and PR them for someone to review.

Also I think that all my changes should go in to 4.0 now. Unless I'm making a bug fix or something urgent, I think my changes are big enough that we should just move them all to 4.0 to keep the current release more stable.

@jjatie jjatie mentioned this pull request Dec 26, 2017
@jjatie
Copy link
Collaborator Author

jjatie commented Dec 26, 2017

Continued in #3136

@jjatie jjatie closed this Dec 26, 2017
@jjatie jjatie deleted the axis-renderer-protocol branch December 26, 2017 04:15
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.

5 participants