influxdata / influxdb-client-swift

InfluxDB (v2+) Client Library for Swift
https://influxdata.github.io/influxdb-client-swift/
MIT License
26 stars 8 forks source link

Ergonomic and idiomatic improvements #24

Closed stuartcarnie closed 3 years ago

stuartcarnie commented 3 years ago

This issue provides a number of suggestions to improve type safety and lead to more idiomatic Swift.

Use a read-only var property accessor

https://github.com/influxdata/influxdb-client-swift/blob/eabb2999f79af7745ab006f253ab2ede5db0585b/Sources/InfluxDBSwiftApis/InfluxDB2API.swift#L52-L54

to:

internal var urlSession { client.session }

Avoid specifying default access level

With few exceptions, internal is the default access level, and therefore does not need to be included. See https://docs.swift.org/swift-book/LanguageGuide/AccessControl.html#ID7 for more information.

Use lazy stored properties for the APIs

https://github.com/influxdata/influxdb-client-swift/blob/5dc1dce36fa5a41538c4ab4572f6d45d3729f50b/Sources/InfluxDBSwiftApis/InfluxDB2API.swift#L68-L70

to:

public lazy var authorizations: AuthorizationsAPI = { AuthorizationsAPI(influxDB2API: self) }()

This avoids creating redundant instances every time the "API" functions are accessed and provides a more fluent, idiomatic API.

NOTE: The definition of the lazy var must be moved out of the extension definition into the class

Use @discardableResult to avoid compiler warnings

https://github.com/influxdata/influxdb-client-swift/blob/e52b98f4f5bb912b545735c6d6437702c61a2688/Sources/InfluxDBSwift/Point.swift#L70

can be annotated with @discardableResult:

@discardableResult public func addTag(key: String?, value: String?) -> Point {

permitting the following:

https://github.com/influxdata/influxdb-client-swift/blob/e52b98f4f5bb912b545735c6d6437702c61a2688/Sources/InfluxDBSwift/Point.swift#L50

to be changed to:

point.addTag(key: tag.0, value: tag.1)

Use sum types for field values:

Accepting Any makes it difficult to determine what values are acceptable. Passing unsupported values will also result in runtime errors:

https://github.com/influxdata/influxdb-client-swift/blob/e52b98f4f5bb912b545735c6d6437702c61a2688/Sources/InfluxDBSwift/Point.swift#L83

Favour enum as a sum type. In addition, these cases match the data types used in Line Protocol and throughout the documentation:

public enum FieldValue {
    case integer(Int)
    case float(Double)
    case boolean(Bool)
    case string(String)
    case unsigned(UInt)
}

public func addField(key: String, value: FieldValue) {
    // ...
}

Rename WritePrecision to TimestampPrecision

This allows us to use the enumeration to describe a timestamp as a tuple (Int, TimestampPrecision) and also the desired output (line protocol) precision.

https://github.com/influxdata/influxdb-client-swift/blob/d9041018e8d4a547df10270217f1a82b6a59cf37/Sources/InfluxDBSwift/InfluxDBClient.swift#L223

Use sum types for timestamp values:

Accepting Any makes it difficult to determine what values are acceptable. Passing unsupported values will also result in runtime errors:

https://github.com/influxdata/influxdb-client-swift/blob/e52b98f4f5bb912b545735c6d6437702c61a2688/Sources/InfluxDBSwift/Point.swift#L96

Favour enum as a sum type, which explicitly states the allowed value types:

public enum TimestampValue {
    case interval(Int, TimestampPrecision)
    case date(Date)
}

public func time(time: TimestampValue) {

It also ensures when using an integer to describe a timestamp, it must carry a precision for the value.

Defer output timestamp precision to serialisation

The desired line protocol write precision should be deferred to the point of serialisation.

Remove:

https://github.com/influxdata/influxdb-client-swift/blob/e52b98f4f5bb912b545735c6d6437702c61a2688/Sources/InfluxDBSwift/Point.swift#L21

Update the toLineProtocol API:

https://github.com/influxdata/influxdb-client-swift/blob/e52b98f4f5bb912b545735c6d6437702c61a2688/Sources/InfluxDBSwift/Point.swift#L107

to include the output timestamp precision:

public func toLineProtocol(precision: TimestampPrecision = defaultWritePrecision, defaultTags: [String: String?]? = nil) throws -> String? {

Prefer implicit member expressions

See: https://docs.swift.org/swift-book/ReferenceManual/Expressions.html#ID394

This reduces the length of the expressions and is considered idiomatic:

https://github.com/influxdata/influxdb-client-swift/blob/e52b98f4f5bb912b545735c6d6437702c61a2688/Sources/InfluxDBSwift/Point.swift#L283

may be rewritten as:

case .s:

Implement CustomStringConvertable to provide description

Deriving from NSObject to provide description is not idiomatic. NSObject opts the class in to the Objective-C runtime, which may not be intentional.

Remove NSObject:

https://github.com/influxdata/influxdb-client-swift/blob/e52b98f4f5bb912b545735c6d6437702c61a2688/Sources/InfluxDBSwift/Point.swift#L11

and description override:

https://github.com/influxdata/influxdb-client-swift/blob/e52b98f4f5bb912b545735c6d6437702c61a2688/Sources/InfluxDBSwift/Point.swift#L119-L122

Add extension which implements CustomStringConvertable:

extension InfluxDBClient.Point: CustomStringConvertible {
    /// Line Protocol from Data Point.
    public var description: String {
        "Point: measurement:\(measurement), tags:\(tags), fields:\(fields), time:\(time ?? "nil")"
    }
}
bednar commented 3 years ago

Hi @stuartcarnie,

I am almost done but I have one question about: Use sum types for field values:.

The FieldValue defined as

public enum FieldValue {
    case integer(Int)
    case float(Double)
    case boolean(Bool)
    case string(String)
    case unsigned(UInt)
}

doesn't supports other types of Int such as Int8, Int16 and so on.

The Swift doesn't allow to use a Protocol for associated enum value. So we aren't able to use UnsignedInteger as a associated enum type (case integer(UnsignedInteger)).

What do you think about add other cases for Int and UInt:

public enum FieldValue {
    case integer(Int)
    case integer8(Int8)
    case integer16(Int16)
    ...
}

or we could drop "direct" support for other types of Int?

Regards

stuartcarnie commented 3 years ago

@bednar I'd recommend you add convenience initialisers to handle conversion, and keep the list of enumerations strictly to the supported types:

extension FieldValue {
    init(value: Int8) {
        self = .integer(Int(value))
    }

    init(value: Int16) {
        self = .integer(Int(value))
    }
}

Then the enumeration can be constructed:

let someValue: Int8 = 5
let val = FieldValue(value: someValue)
bednar commented 3 years ago

@stuartcarnie thanks it looks good 👍

bednar commented 3 years ago

@stuartcarnie

Defer output timestamp precision to serialisation

The precision on the Point is for planning supports of batching write. If the client will be use as a gateway then every point could have different precision.