ChartsOrg / ChartsOrg/Charts

Swift 4, Obj-C compatibility, access control, naming, and Swift convention

Open
#2,619 8 comments 0 reactions 0 assignees View on GitHub
Dominant language
Swift
Stars
28k
Forks
6k
PR merge metrics
No merged PRs in 30d

Description

With a new major release of Swift, it seems as good a time as any to reevaluate some key aspects of Charts. First the changes that Swift 4 brings requires no changes to the Charts codebase — outside of some naming that the migrator takes care of.

## Objective-C Compatibility
The biggest thing I'd like to see is a move away from Objective-C compatibility. This is a huge change, but it would allow us to get rid of all the `NSObject` subclasses and in many cases replace them with Structs/Enums (e.g. `Fill` can be rewritten with half the code as an enum with associated values). It could also allow us to turn the Charts data source (`ChartData`, `ChartDataSet`, `ChartDataEntry`) into generic value types which would remove the need for all the casting that's currently required and should improve performance with large data sets, or at the very least, memory usage. At the very least, being able to strictly use Swift convention will drastically reduce the codebase.

My thought is that Charts 4 be the first version to Obj-C drop compatibility, with Charts 3.x only receiving bug fixes. There are many things we can do to make it easier to migrate from, and maintain Charts 3.x (e.g. forbidding the use of `isEqual()`, using `==` instead, and providing identical overrides to both methods), but I don't think this post is the place to discuss implementation.

## Access Control and Naming
When I first started using Charts, I found it confusing which API to use (e.g. `pinchZoomEnabled` vs `isPinchZoomEnabled`). In most — I think all — cases, only one of them is mutable so it's easy to figure out, but it's still an issue. Part of the problem is naming, and the other is access control. Virtually the entire API is public or open, but little, if any, of it actually should be overridable, and many things don't need to be visible to the user. I would like to see a reevaluation of access control, and get rid of internal variables where not necessary (e.g. `_pinchZoomEnabled`, `pinchZoomEnabled`, and `isPinchZoomEnabled`). I would also like to see some function/property renaming to be in line with the [Swift API Guidelines](https://swift.org/documentation/api-design-guidelines/).

## Swift Convention
There are many areas where it makes sense to adopt some standard Swift protocols like `Equatable`, `Collection` (and related), etc. I would like to see more of these implemented. Many algorithms can be simplified to much less code (e.g. many if-else statements can be replaced with `min(_,_)`/`max(_,_)`, and many loops can be replaced with `map`, `filter`, or `reduce`).

There's a lot here, and as @liuxuan30 points out, we're a bit short handed, but I would be happy do much of the work (especially the move away from Obj-C), but first we need discussion and decisions to be made.

Contributor guide

Open the contributing guide

Research direction

Read the issue sections on Objective-C compatibility, access control and naming, and Swift convention, then inspect ChartData, ChartDataSet, ChartDataEntry and the Fill type in the Charts codebase. Review the current public API and existing Swift 3.x compatibility constraints before proposing a scoped plan. Done requires maintainer decisions on the migration scope and a separately defined implementation task.

Written by the indexing model from the issue text.

Assessment

Tech stack
objective-c, swift
Domain
mobile
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
20/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.