Feature/response header in did finish upload delegate #167
Conversation
|
@Acconut @donnywals |
Sources/TUSKit/TUSClient.swift
Outdated
| @@ -16,7 +16,7 @@ public protocol TUSClientDelegate: AnyObject { | |||
| /// TUSClient is starting an upload | |||
| func didStartUpload(id: UUID, context: [String: String]?, client: TUSClient) | |||
| /// `TUSClient` just finished an upload, returns the URL of the uploaded file. | |||
| func didFinishUpload(id: UUID, url: URL, context: [String: String]?, client: TUSClient) | |||
| func didFinishUpload(id: UUID, url: URL, responseHeaders: [String: String]?, context: [String: String]?, client: TUSClient) | |||
There was a problem hiding this comment.
Adding support for response headers this way makes this PR a breaking change which I want to avoid.
We could have both versions of didFinishUpload with and without the responseHeaders defined on this protocol. Both would need to be called from the places where we call delegate?.didFinishUpload which would be error prone so expanding the test suite to verify we always call both would be desirable.
If you have other ideas to implement this as a non-breaking change, I'm all ears 👍🏼
There was a problem hiding this comment.
I've made the above change making func didFinishUpload(id: UUID, url: URL, context: [String: String]?, client: TUSClient, responseHeaders: [String: String]?) an optional function and called both the functions in a private function.
In the mock test, I've just added a skeleton that confirms to the optional delegate method. Could you please suggest me to how to make the test case work with the changes?
There was a problem hiding this comment.
I'll try and take a detailed look at that later this week, possibly next week. I'm currently on vacation so only have very short bursts of time to look at PRs and do bits of work.
There was a problem hiding this comment.
Sure. Have a nice vacation.
Will be looking forward for your feedback after your vacation.
| @@ -40,6 +40,11 @@ public extension TUSClientDelegate { | |||
| func progressFor(id: UUID, context: [String: String]?, progress: Float, client: TUSClient) { | |||
| // Optional | |||
| } | |||
|
|
|||
| /// `TUSClient` just finished an upload, returns the URL of the uploaded file along with `responseHeaders` from server. | |||
| func didFinishUpload(id: UUID, url: URL, context: [String: String]?, client: TUSClient, responseHeaders: [String: String]?) { | |||
There was a problem hiding this comment.
Sorry, I actually think this would still be breaking since if I conform to the delegate protocol I must implement all methods required by the protocol. This slipped my mind earlier.
Maybe we can provide a default empty implementation in a protocol extension for this method so that existing users of the SDK wouldn't have to do any work after updating?
I do wonder if we could mark a protocol method as deprecated. If we can, we could deprecate the old one and tell clients to implement the new method
Addition of didFinishUpload with responseHeader in TUSClientDelegate protocol definition.
|
Hi, should we close this? |
|
Will close due to inactivity |
No description provided.