ChartsOrg / Charts

Beautiful charts for iOS/tvOS/OSX! The Apple side of the crossplatform MPAndroidChart.
Apache License 2.0
27.56k stars 5.99k forks source link

Removing weird `is...` getters? #1752

Open danielgindi opened 7 years ago

danielgindi commented 7 years ago

When first implementing this library, ObjC compatibility was in mind all the time. And so, is getters were written too. The problem is, there was no feature like getter=... in Swift. (There's a relatively new option to use @objc(is...) on the getter, but that also requires declaring an additional private variable)

Now it seems that as Swift is getting more mature, there is not going to be such a feature. Apple also tends to write Swift properties without the is prefix, unless there's only a getter.

I want to hear your thoughts, pros and cons, before making a decision on this. Shoot.

danielgindi commented 7 years ago

@petester42

pmairoldi commented 7 years ago

I'm all for normailizing the names with swift conventions. In addition you can have a public getter and a private setting for the same property in swift. https://www.natashatherobot.com/swift-magic-public-getter-private-setter/

danielgindi commented 7 years ago

I actually didn't understand how would a private getter assist us in this case...

pmairoldi commented 7 years ago

No no. The setter would be private so you don't need a separate private property to have only a public getter. So we can use the @objc tag on that property to change the name.

danielgindi commented 7 years ago

Still I don't get how that would help? Without private setter, for drawValuesEnabled, we would:

private var _drawValuesEnabled: Bool = false
open var drawValuesEnabled: Bool
{
  @objc(isDrawValuesEnabled) get { return _drawValuesEnabled }
  set { _drawValuesEnabled = newValue }
}

Which will achieve @objc compatibility.

Also I'm not sure about this yet, as Apple did change hidden to isHidden on UIView in Swift version. Can't find a consistent rule, but I think that just hidden sounds more Swifty, and isHidden more objc...

pmairoldi commented 7 years ago

What I am talking about would look like this:

@objc(isDrawValuesEnabled)
open private(set) var drawValuesEnabled: Bool

No need for a separate _drawValuesEnabled that way you can change the name and have only the getter accessible.

For the case you just mentioned wouldn't it just be drawValuesEnabled as a public property? why do you even need the private variable like so:

@objc(isDrawValuesEnabled)
open var drawValuesEnabled: Bool
danielgindi commented 7 years ago

If the setter is private then what's the point? We need a public setter...

The private variable is because we do @objc(isDrawValuesEnabled) get, we change specifically the name of the getter, not the whole property

pmairoldi commented 7 years ago

Ya I get your point now. I think the way apple is pushing is to have it be as readable as possible. So we could just rename drawValueEnabled to canDrawValues and not worry about the objc stuff.

danielgindi commented 7 years ago

Yeah but that's one property, there are lots of properties to worry about, a lot of names. That is why I thought about just removing the is getters completely

pmairoldi commented 7 years ago

That sounds good to me.

liuxuan30 commented 7 years ago

seems swift likes isHidden style.. I checked several places like isContinuous, isKeyWindow

jjatie commented 7 years ago

Swift's API guidelines are clear on this,

Uses of Boolean methods and properties should read as assertions about the receiver when the use is nonmutating, e.g. x.isEmpty, line1.intersects(line2).