Resolves #170 - Return error on monitor disconnection #175

bartoszstelmaszuk · 2017-10-26T10:37:04Z

Resolves #170 so the library returns error on monitorDisconnection to the user

siejkowski · 2017-10-26T10:50:42Z

Source/BluetoothManager.swift

@@ -255,8 +255,8 @@ public class BluetoothManager {
    /// - parameter peripheral: The `Peripheral` to which the `BluetoothManager` is either trying to
    /// connect or has already connected.
    /// - returns: `Single` which emits next event when peripheral successfully cancelled connection.
-    public func cancelPeripheralConnection(_ peripheral: Peripheral) -> Single<Peripheral> {
-        let observable = Observable<Peripheral>.create { [weak self] observer in
+    public func cancelPeripheralConnection(_ peripheral: Peripheral) -> Observable<(Peripheral, BluetoothError?)> {


Maybe we could introduce a wrapper type to indicate that the peripheral might be returned without error?

What do you mean by that? Optional type indicates that there may or may not be an error related to disconnection. How would you name your wrapper type? Will it be an enum?

Looking through library i found that in most cases a tuple is used instead of wrapper type. I decided to follow the current style. Don't you think that optionality on BluetoothError indicates enough that the observable can return without error?

siejkowski · 2017-10-26T10:52:04Z

Source/BluetoothManager.swift

@@ -265,7 +265,7 @@ public class BluetoothManager {
            strongSelf.centralManager.cancelPeripheralConnection(peripheral.peripheral)
            return disposable
        }
-        return ensure(.poweredOn, observable: observable).asSingle()


Is there a reason for dropping the Single here? Can we emit multiple events now?

Good catch! Already repairing it.

siejkowski · 2017-10-26T10:57:21Z

Source/BluetoothManager.swift

+    public func monitorDisconnection(for peripheral: Peripheral) -> Observable<(Peripheral, BluetoothError?)> {
+        return monitorPeripheral(
+            on: delegateWrapper.rx_didDisconnectPeripheral.map { (_, error) in
+                throw BluetoothError.peripheralDisconnectionFailed(peripheral, error)


I might not read it correctly, but is there a reason for why you're ignoring the peripheral returned from peripheralAction observable here? It seems to me that the logic deciding which instance should be returned in the error should be kept in one place and monitorPeripheral is better place in my opinion. Right now it's been done twice: in monitorDisconnection method and monitorPeripheral method.

This code is not behaving correctly. We shouldn't throw error there.

+1. In the issue I said that error should be emitted in the resulting observable next events, not as an error on stream.

corrected throwing. It should be mapped as you mentioned.

Cierpliwy · 2017-10-26T11:19:47Z

Source/BluetoothManager.swift

@@ -255,8 +255,8 @@ public class BluetoothManager {
    /// - parameter peripheral: The `Peripheral` to which the `BluetoothManager` is either trying to
    /// connect or has already connected.
    /// - returns: `Single` which emits next event when peripheral successfully cancelled connection.
-    public func cancelPeripheralConnection(_ peripheral: Peripheral) -> Single<Peripheral> {
-        let observable = Observable<Peripheral>.create { [weak self] observer in
+    public func cancelPeripheralConnection(_ peripheral: Peripheral) -> Observable<(Peripheral, BluetoothError?)> {


What do you mean by that? Optional type indicates that there may or may not be an error related to disconnection. How would you name your wrapper type? Will it be an enum?

Cierpliwy · 2017-10-26T11:22:35Z

Source/BluetoothManager.swift

+        return monitorPeripheral(
+            on: delegateWrapper.rx_didConnectPeripheral.map { ($0, nil) },
+            peripheral: peripheral
+        ).map { $0.0 }


Are we creating tuple to just drop it at the end?

Yes, it is to use monitorPeripheral function in both connection and disconnection functions.

I'm not convinced to this — It's right now kind of artificial way to do this generalization. Could we find some other approach to implement this? This tuple creation reason is complicated and deserves comment at least. The best would be to find other way or just drop helper function.

Cierpliwy · 2017-10-26T11:26:12Z

Source/BluetoothError.swift

@@ -78,6 +79,8 @@ extension BluetoothError: CustomStringConvertible {
            return "Connection error has occured: \(err?.localizedDescription ?? "-")"
        case let .peripheralDisconnected(_, err):
            return "Connection error has occured: \(err?.localizedDescription ?? "-")"
+        case let .peripheralDisconnectionFailed(_, err):
+            return "Connection error has occured: \(err?.localizedDescription ?? "-")"


According to documentation disconnection cannot fail. So there shouldn't be any error related to that.

Cierpliwy · 2017-10-26T11:26:40Z

Source/BluetoothManager.swift

+    public func monitorDisconnection(for peripheral: Peripheral) -> Observable<(Peripheral, BluetoothError?)> {
+        return monitorPeripheral(
+            on: delegateWrapper.rx_didDisconnectPeripheral.map { (_, error) in
+                throw BluetoothError.peripheralDisconnectionFailed(peripheral, error)


This code is not behaving correctly. We shouldn't throw error there.

Cierpliwy · 2017-10-26T11:27:35Z

Source/BluetoothManager.swift

    }

-    func monitorPeripheral(on peripheralAction: Observable<CBPeripheral>, peripheral: Peripheral)
-        -> Observable<Peripheral> {
+    func monitorPeripheral(on peripheralAction: Observable<(CBPeripheral, BluetoothError?)>, peripheral: Peripheral)


I'm not sure if this function is worth to share it between connected/disconnected events.

in my opinion it is. If we would like to separate it instead of two easy maps we would have to implement logic of filtering and mapping on stream of pheripheralAction with ensuring poweredOn state in both function monitorConnection and monitorDisconnection

Cierpliwy · 2017-10-26T11:27:54Z

Source/Peripheral.swift

@@ -92,7 +92,7 @@ public class Peripheral {
    /// that physical connection will be closed immediately as well and all pending commands will not be executed.
    ///
    /// - returns: `Single` which emits next event when peripheral successfully cancelled connection.
-    public func cancelConnection() -> Single<Peripheral> {
+    public func cancelConnection() -> Observable<(Peripheral, BluetoothError?)> {


Why observable?

Kacper20

Couple of more comments. We're getting there :)

Kacper20 · 2017-10-26T22:05:51Z

Source/Peripheral.swift

@@ -92,7 +92,7 @@ public class Peripheral {
    /// that physical connection will be closed immediately as well and all pending commands will not be executed.
    ///
    /// - returns: `Single` which emits next event when peripheral successfully cancelled connection.
-    public func cancelConnection() -> Single<Peripheral> {
+    public func cancelConnection() -> Single<(Peripheral)> {


We don't need parentheses here.

Kacper20 · 2017-10-26T22:07:50Z

Source/BluetoothManager.swift

@@ -265,7 +265,7 @@ public class BluetoothManager {
            strongSelf.centralManager.cancelPeripheralConnection(peripheral.peripheral)
            return disposable
        }
-        return ensure(.poweredOn, observable: observable).asSingle()
+      return ensure(.poweredOn, observable: observable).asSingle().map { $0.0 }


I would move map to the new line for better readability

Kacper20 · 2017-10-26T22:29:55Z

Source/BluetoothManager.swift

-    /// - Returns: Observable which emits next events when `peripheral` was disconnected.
-    public func monitorDisconnection(for peripheral: Peripheral) -> Observable<Peripheral> {
-        return monitorPeripheral(on: delegateWrapper.rx_didDisconnectPeripheral.map { $0.0 }, peripheral: peripheral)
+    /// - Returns: Observable which emits next events when `peripheral` was disconnected and error which is


I would say: Returns: Observable which emits next events when `Peripheral` instance was disconnected. It provides optional error which may contain more information about the cause of the disconnection if it wasn't the `cancelConnection` call

Kacper20 · 2017-10-26T22:31:43Z

Source/BluetoothManager.swift

-        return monitorPeripheral(on: delegateWrapper.rx_didDisconnectPeripheral.map { $0.0 }, peripheral: peripheral)
+    /// - Returns: Observable which emits next events when `peripheral` was disconnected and error which is
+    /// a disconnection reason when it was caused by 'peripheral' otherwise it's nil.
+    public typealias DisconnectionReason = Error?


type alias should be above the documentation comments. Right now docs refer to type alias not to a function. Additionaly it should be DisconnectionReason = Error. Then in interface user sees immediately that it's optional — without looking at type alias definition.

Kacper20 · 2017-10-26T22:32:42Z

Source/BluetoothManager.swift

-    func monitorPeripheral(on peripheralAction: Observable<CBPeripheral>, peripheral: Peripheral)
-        -> Observable<Peripheral> {
+    func monitorPeripheral(on peripheralAction: Observable<(CBPeripheral, DisconnectionReason)>, peripheral: Peripheral)
+            -> Observable<(Peripheral, Error?)> {


We could also use DisconnectionReason here instead of an Error

Kacper20 · 2017-10-26T22:35:45Z

Source/BluetoothManager.swift

+        return monitorPeripheral(
+            on: delegateWrapper.rx_didConnectPeripheral.map { ($0, nil) },
+            peripheral: peripheral
+        ).map { $0.0 }


I'm not convinced to this — It's right now kind of artificial way to do this generalization. Could we find some other approach to implement this? This tuple creation reason is complicated and deserves comment at least. The best would be to find other way or just drop helper function.

…thod

…pealias instead of BluetoothError

bartoszstelmaszuk assigned Kacper20 Oct 26, 2017

bartoszstelmaszuk changed the base branch from master to 5.0-dev October 26, 2017 10:40

siejkowski reviewed Oct 26, 2017

View reviewed changes

Cierpliwy reviewed Oct 26, 2017

View reviewed changes

bartoszstelmaszuk changed the title ~~Return error on monitor disconnection~~ Resolves #170 - Return error on monitor disconnection Oct 26, 2017

Kacper20 suggested changes Oct 26, 2017

View reviewed changes

bartoszstelmaszuk and others added 6 commits October 27, 2017 13:56

return error on monitorDisconnection to the user

7403fc6

reverting change of returned type from Single to Observable

619fad5

review fixes removing added error case and proper mapping delegate me…

aa9f72f

…thod

review fixes to express disconnection error as DisconnectionReason ty…

9501a1d

…pealias instead of BluetoothError

review fixes

40b2bbb

removed monitorPeripheral function in BluetoothManager

57a1192

bartoszstelmaszuk force-pushed the return_error_on_monitor_disconnection branch from 62b3a11 to 57a1192 Compare October 27, 2017 11:57

Kacper20 approved these changes Oct 27, 2017

View reviewed changes

Kacper20 merged commit db8c4bc into Polidea:5.0-dev Oct 27, 2017

Kacper20 mentioned this pull request Oct 30, 2017

[5.0] monitorDisconnection on Peripheral instance should return the error to the user #170

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolves #170 - Return error on monitor disconnection #175

Resolves #170 - Return error on monitor disconnection #175

bartoszstelmaszuk commented Oct 26, 2017 •

edited

siejkowski Oct 26, 2017

Cierpliwy Oct 26, 2017

bartoszstelmaszuk Oct 26, 2017

siejkowski Oct 26, 2017

bartoszstelmaszuk Oct 26, 2017

siejkowski Oct 26, 2017 •

edited

Cierpliwy Oct 26, 2017

Kacper20 Oct 26, 2017

bartoszstelmaszuk Oct 26, 2017

Cierpliwy Oct 26, 2017

Cierpliwy Oct 26, 2017

bartoszstelmaszuk Oct 26, 2017

Kacper20 Oct 26, 2017

Cierpliwy Oct 26, 2017

Kacper20 Oct 26, 2017

Cierpliwy Oct 26, 2017

Cierpliwy Oct 26, 2017

bartoszstelmaszuk Oct 26, 2017

Cierpliwy Oct 26, 2017

Kacper20 left a comment

Kacper20 Oct 26, 2017

Kacper20 Oct 26, 2017

Kacper20 Oct 26, 2017

Kacper20 Oct 26, 2017

Kacper20 Oct 26, 2017

Kacper20 Oct 26, 2017

Resolves #170 - Return error on monitor disconnection #175

Resolves #170 - Return error on monitor disconnection #175

Conversation

bartoszstelmaszuk commented Oct 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siejkowski Oct 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kacper20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartoszstelmaszuk commented Oct 26, 2017 •

edited

siejkowski Oct 26, 2017 •

edited