SDK V2 #28
SDK V2 #28
Conversation
WeTransfer/Models/Board.swift
Outdated
| import Foundation | ||
|
|
||
| /// Desribes a single board to be created, adding files to and uploading files from. Used as an identifier between each request to be made and a local representation of the server-side board. | ||
| /// Files should be added through the approapriate addFiles method |
WeTransfer/Models/Board.swift
Outdated
|
|
||
| /// Desribes a single board to be created, adding files to and uploading files from. Used as an identifier between each request to be made and a local representation of the server-side board. | ||
| /// Files should be added through the approapriate addFiles method | ||
| public final class Board: Transferrable { |
There was a problem hiding this comment.
Transferrable -> Transferable
| public let description: String? | ||
|
|
||
| /// References to all the files added to the board. Files can be added with the public method on the WeTransfer struct | ||
| public private(set) var files: [File] = [] |
There was a problem hiding this comment.
Boards can contain links as well, which are not files. Rather call this Content
There was a problem hiding this comment.
Links wil be added when addressing #8, for now boards and transfer only support files.
|
|
||
| init(name: String, description: String?) { | ||
| self.name = name | ||
| self.description = description |
There was a problem hiding this comment.
It's just tabs, just like in the rest of the project. Will be addressed in future PR
| /// Number of multiparts (chunks) and upload identifier for uploading | ||
| let meta: Meta | ||
| let multipart: Multipart |
There was a problem hiding this comment.
Isn't this upload info? Multipart is not really describing to me
There was a problem hiding this comment.
This will be picked up when addressing #16. For now all responses are exactly as returned by the API (minus snake case of course)
There was a problem hiding this comment.
You can adjust the name here, right? It's really simple to change just a parameter name.
There was a problem hiding this comment.
Oh you mean the struct name, that is indeed possible. Done!
| /// Response from complete upload request | ||
| struct CompleteUploadResponse: Decodable { | ||
| struct CompleteTransferFileUploadResponse: Decodable { |
There was a problem hiding this comment.
Rather define a generic EmptyResponse instead so it can be reused
There was a problem hiding this comment.
Done, great suggestion.
| operation.onResult = { result in | ||
| DispatchQueue.main.async { | ||
| completion(result) | ||
| } | ||
| } | ||
|
|
||
| // Externally create the board if not already in the queue | ||
| if board.identifier == nil && client.operationQueue.operations.first(where: { $0 is CreateBoardOperation }) == nil { |
There was a problem hiding this comment.
Isn't this identifier the publicIdentifier? we can make this line more describing by making something like board.isShared. Now I'm guessing, so we only do this when the identifier is nil? Like, why only then?
There was a problem hiding this comment.
Mentioned and now appended in #13, good stuff 👍
Generated by 🚫 Danger |
|
|
||
| override func execute() { | ||
| guard board.identifier == nil else { | ||
| self.finish(with: .success(board)) |
| self.file = file | ||
| self.chunkIndex = chunkIndex | ||
| } | ||
|
|
||
| override func execute() { | ||
| guard let fileIdentifier = file.identifier, let uploadIdentifier = file.multipartUploadIdentifier else { | ||
| guard let containerIdentifier = container.identifier, let fileIdentifier = file.identifier else { | ||
| self.finish(with: .failure(Error.fileNotYetAdded)) |
| endpoint = .requestTransferUploadURL(transferIdentifier: containerIdentifier, fileIdentifier: fileIdentifier, chunkIndex: chunkIndex) | ||
| } else if container is Board { | ||
| guard let multipartIdentifier = file.multipartUploadIdentifier else { | ||
| self.finish(with: .failure(Error.fileNotYetAdded)) |
| @@ -9,7 +9,7 @@ | |||
| import Foundation | |||
|
|
|||
| /// Handles the uploading of all files -- that are not uploaded yet -- in the provided transfer. Creates an operation queue with an `UploadFileOperation` for each file to be uploaded. | |||
| final class UploadFilesOperation: ChainedAsynchronousResultOperation<Transfer, Transfer>, URLSessionDataDelegate { | |||
| final class UploadFilesOperation<Container>: ChainedAsynchronousResultOperation<Container, Container>, URLSessionDataDelegate where Container: Transferable { | |||
There was a problem hiding this comment.
You can just use Container: Transferable, right? And shouldn't we name it like transferrable with 2 rs?
There was a problem hiding this comment.
It was actually the other way around, I typed it with 2 rs originally but that's not the correct spelling.
| override func tearDown() { | ||
| super.tearDown() | ||
| TestConfiguration.resetConfiguration() | ||
| } |
There was a problem hiding this comment.
Don't you want to create a base test class? quite some duplicate code everywhere now with the same tearDown and setUp
|
|
||
| var observation: NSKeyValueObservation? | ||
|
|
||
| func testFileUpload() { |
There was a problem hiding this comment.
This test seems to do more than one thing indeed. You might want to split this up
| /// Number of multiparts (chunks) and upload identifier for uploading | ||
| let multipart: Multipart | ||
| /// Upload info for the file | ||
| let multipart: UploadInfo |
There was a problem hiding this comment.
I guess it's good to also rename the parameter name to uploadInfo.
There was a problem hiding this comment.
Yes, but I’m not going to do that know. All changed to Decodable structs will be addressed when fixing issue #16
There was a problem hiding this comment.
Why not? It's really easy. Find +replace or using the refactor mechanism in Xcode. It only makes your life easier with #16. I don't really get why you don't want to do it here actually 🤔
There was a problem hiding this comment.
But it’s a response from the server and if I add the CodingKeys here, I’d rather do those all at once
Big update again. It was too complicated to break this down into separate branches as most changes were touching too many files.
What happened?
As with WeTransfer API V2, the SDK now allows the implementer to use both Boards and Transfers with their respective unique functionality.
What to look for
Transferableprotocol make sense?While this PR might lightly address some, please look at the standing issues before commenting unrelated structural quirks.