Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

Organize exceptions thrown #84

Merged
merged 1 commit into from
Sep 15, 2020
Merged

Organize exceptions thrown #84

merged 1 commit into from
Sep 15, 2020

Conversation

twyatt
Copy link
Member

@twyatt twyatt commented Aug 27, 2020

IOException chosen for failures where an I/O operation did not succeed. It is expected that library users will catch these exceptions and decide if operation should be retried or if more extreme measures should be taken (e.g. reconnect prior to retrying).

Closes #81.

core

Class Base Exception
RemoteException Exception
ConnectionLostException IOException
GattStatusException IOException
OutOfOrderGattCallbackException IllegalStateException
  • OutOfOrderGattCallbackException should never occur. This failure is not recoverable and the behavior of future requests is undefined. Library user should allow app to crash or re-establish connect (to reset callback state). If this exception is ever encountered it should be reported so it may be investigated.
  • RemoteException occurs when the Android system fails to execute a request, it is unlikely that retrying the operation will succeed until the user intervenes (e.g. turns on BLE).

keep-alive

Class Base Exception
NotReadyException IOException

throw

Class Base Exception
GattStatusException (core) IOException
RemoteException Exception
ConnectionLostException IOException

📓 Notes

Since SharedFlow is not yet officially released, this branch was developed against internally built Coroutines artifacts; this PR will be merged into internal branch (see #86 for details).

@twyatt twyatt force-pushed the twyatt/exceptions branch from e24dd72 to 62eb3f0 Compare August 27, 2020 21:07
@twyatt twyatt changed the title Throw GattStatusException from throw extension functions Organize exceptions thrown Aug 28, 2020
@twyatt twyatt added this to the 1.0.0 milestone Aug 28, 2020
@twyatt twyatt force-pushed the twyatt/exceptions branch from acbd52e to 994fbc9 Compare August 28, 2020 22:00
@twyatt twyatt modified the milestones: 1.0.0, 0.8.0 Sep 12, 2020
@twyatt twyatt marked this pull request as ready for review September 12, 2020 06:51
@twyatt twyatt requested review from a team, sdonn3 and cedrickcooke and removed request for a team September 12, 2020 06:51
Comment on lines +48 to +53
class GattStatusException(message: String?) : IOException(message) {
constructor(
status: GattStatus,
prefix: String
) : this("$prefix failed with status ${status.asGattStatusString()}")
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these constructors be internal like the other exceptions?

Copy link
Member Author

@twyatt twyatt Sep 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but...
This exception is also thrown from throw module.

I tried utilizing friend-paths (to give throw module access to internal definitions in core) but could not get it working. 😢

Spent about 3 hours trying to get the friend paths working before giving up and just accepting this little bit of cruft on the public API surface.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really wish Kotlin had way more visibility modifiers. Something like Android's @RestrictTo but at a language level.

@twyatt twyatt requested a review from cedrickcooke September 14, 2020 19:12
Base automatically changed from twyatt/SharedFlow to internal September 14, 2020 20:38
@sdonn3
Copy link
Contributor

sdonn3 commented Sep 15, 2020

lgtm!

@twyatt twyatt merged commit 3a8fa68 into internal Sep 15, 2020
@twyatt twyatt deleted the twyatt/exceptions branch September 15, 2020 17:34
twyatt added a commit that referenced this pull request Oct 15, 2020
@twyatt
Copy link
Member Author

twyatt commented Oct 15, 2020

Small update: This PR was brought into develop via a fast-forwarding of develop to internal (that was dropped). So some slight modifications of the squashed commit may exist in the final commit that landed in develop.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants