Updated all Bool ivars to get rid of special Obj-C backing variables.
#2687
Conversation
- There is no more confusion for `Bool` ivars in the exposed API, as there is only one exposed variable (i.e. no `pinchToZoomEnabled` and `isPinchToZoomEnabled`). - All backing variables are `private`, not `fileprivate`. The backing variables should only be called in the getter/setter of the public ivar. - All names internal to Charts have been updated for Swift naming conventions - All names external to Charts have obj-c setters with the appropriate obj-c naming convention - Almost all `open` access control on `Bool` ivars has been changed to `public`. This is what they should have been in the first place, as overriding all but a few of these ivars would result in unintended consequences. - Minor update to demos to support changes.
|
This would be a breaking change. I think we should hold off on this a bit until we make other breaking changes. If you have ideas on how to improve this to not make it a breaking change I'd be glad to hear them. |
|
Unfortunately this needs be a breaking change. As you commented on a few other of my changes, it makes sense to wait until Swift 4/Charts (4?) is released. |
Codecov Report
@@ Coverage Diff @@
## master #2687 +/- ##
==========================================
+ Coverage 19.77% 19.79% +0.02%
==========================================
Files 113 113
Lines 13663 13644 -19
==========================================
- Hits 2702 2701 -1
+ Misses 10961 10943 -18
Continue to review full report at Codecov.
|
|
Can we either merge this, or create a branch for breaking changes (3.1 or 4.0 I'm guessing?) |
|
@petester42 This should be 4.0.0 I guess. |
|
@petester42 While this change works, it would be ideal to be able to just declare This removes the custom setter name |
|
Seems reasonable to me to do this, for 4.0. One of the main goals of this library is being compatible with its Android counterpart, so we need to keep the naming close (while doing our best to conform to Swift standards). |
|
@danielgindi Great. I will reevaluate the names this week (maybe today) to try and get the most consistency with android and Swift possible. Do you think it would be appropriate to drop the custom objc setters as I noted above? It would clean up the code A LOT as there wouldn't need to be a private backing var for every boolean property. |
|
Wherever we can drop the custom objc getter/setters - it's best. They are just another thing that we need to manage and we don't want that... |
|
That makes me recall this project is started with Swift 1.1, at that time, we have to do something weird from what you see now :) |
|
This is a huge codebase change, and as you can see many conflicts are building up. I will resubmit this to 4.0.0 later (likely as in pieces as part of other refactors) to make this more manageable. |
Boolivars in the exposed API, as there is only one exposed variable (i.e. nopinchToZoomEnabledandisPinchToZoomEnabled).private, notfileprivate. The backing variables should only be called in the getter/setter of the public ivar.openaccess control onBoolivars has been changed topublic. This is what they should have been in the first place, as overriding all but a few of these ivars would result in unintended consequences.There are still some names that I would like to be changed, but I left them alone as that is outside the scope of this PR.