search

ably / ably-asset-tracking-swift

iOS client SDKs for the Ably Asset Tracking service.
Apache License 2.0
9 stars 6 forks source link

Feature/589 ably wrapper no auto connect #596

Closed AndyTWF closed 1 year ago

AndyTWF commented 1 year ago

Resolves #589

lawrence-forooghian commented 1 year ago

Do we also need a "Resolves #xyz" in the PR description?

AndyTWF commented 1 year ago

I've taken an initial look at this, but I'm currently a bit confused about the scope of this PR. I thought the idea was that we were just going to, essentially, replace a single method call with two. But there seems to also be stuff about stopping the connection if we fail to start it, which seems unrelated to the task at hand. Is there a reason it needs to be here, would it not be better off being done when we do the workers?

Yeah you're right, it was probably a bit of out of scope to make the connection stop after failure to connect.

As I was dealing with it in startConnection, I decided to refactor with the stopConnection method slightly to take a consistent approach with the semaphore and thus avoid the leakage problems. It was a small hop after that to put the method calls in to shutdown the connection.

I can remove them again if you think that would be better - though I think we should keep the refactor in stopConnection as that's now consistent and improved behaviour?