Add support for creating and restoring data set#4
Conversation
darinkrauss
commented
Mar 17, 2020
darinkrauss
commented
- Logout when service is deleted
- Add bundle semantic version
- Logout when service is deleted - Add bundle semantic version
rickpasetto
left a comment
rickpasetto
left a comment
There was a problem hiding this comment.
Looks ok so far (would love to see some unit tests), but I had some questions, if you have a chance...
| self.error = error | ||
| case .success(let dataSets): | ||
| if !dataSets.isEmpty { | ||
| self.dataSetId = dataSets.first?.uploadId |
There was a problem hiding this comment.
Sorry I'm not familiar with the Tidepool API, but is it customary to only grab the first dataSet? What if there are more?
There was a problem hiding this comment.
This is searching for a data set that is specific to Tidepool Loop. There should only be one of them at any time. So, grabbing the first one is appropriate here. However, I will add a check and log if there is more than one.
There was a problem hiding this comment.
Sure, or a comment is fine. Thanks.
| self.session = nil | ||
|
|
||
| DispatchQueue.global(qos: .background).async { | ||
| self.tapi.logout(session: session) { _ in } |
There was a problem hiding this comment.
Sorry, again, I'm not sure I'm understanding the flow here, but, why does completeDelete logout? Is this method intended to delete the data set or delete the "service"?
There was a problem hiding this comment.
completeDelete is invoked when the service is being deleted to do any final clean before all of the service's settings are destroyed. A logout is appropriate in this case.
|
@rickpasetto The latest commit adds logging if there was more than one data set found. Would you please review? |
rickpasetto
left a comment
rickpasetto
left a comment
There was a problem hiding this comment.
One nit, otherwise LGTM.
| case .success(let dataSets): | ||
| if !dataSets.isEmpty { | ||
| if dataSets.count > 1 { | ||
| self.log.default("Found multiple matching data sets; expected zero or one") |
There was a problem hiding this comment.
definitely a [nit] : I would've expected this to be log.error...but this is fine as-is.
- Change multiple data sets log from default to error
|
@rickpasetto I changed the log |
…-service Remote PR Set #2: Introduce RemoteCommands
3 participants
