Feature/response header in did finish upload delegate#167
Conversation
|
@Acconut @donnywals |
| 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.
| } | ||
|
|
||
| /// `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 |
|
Why didn't we end up implementing this? We kinda need this responseHeaders. An empty protocol extension implementation should fix the breaking protocol changes. |
|
@LefterisHaritou given that there was no activity on the PR to resolve breaking changes we closed the issue (and no further customer requests were submitted asking for this feature). You're more than welcome to pick this one up and implementing your suggestion and I'm happy to review and merge this. |
|
I can make the change and push it. Thanks. |
|
@donnywals, I'm unable to reopen this PR from my side as the reopen button isn't available. I have rebased the code to adapt to recent changes, made some updates, and pushed them to my forked branch. Additionally, as you suggested, I've marked the old function as deprecated. Could you please check if you can reopen the PR from your side? |
|
@srvarma7 I just reopened the PR. |
srvarma7
force-pushed
the
feature/response-header-in-didFinishUpload-delegate
branch
from
6a1a13c to
262c1c5
Compare
srvarma7
force-pushed
the
feature/response-header-in-didFinishUpload-delegate
branch
from
262c1c5 to
cfad9de
Compare
5 participants
No description provided.