New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature - Destinations Closures, Download Options and Top-Level APIs #1462
Conversation
cc @jshier and @kcharwood for review. |
c46bc01
to
652536c
Compare
3125beb
to
f2ad0db
Compare
@@ -148,65 +148,74 @@ public func request(_ urlRequest: URLRequestConvertible) -> DataRequest { | |||
/// Creates a `DownloadRequest` using the default `SessionManager` to retrieve the contents of a URL based on the | |||
/// specified `urlString`, `method`, `parameters`, `encoding`, `headers` and save them to the `destination`. | |||
/// | |||
/// If `destination` is not specified, the contents will remain in the temporary location determined by the | |||
/// URL session. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be more specific and say by the underlying URLSession
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Will change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 66fc1a8.
@cnoon Reviews finished, just some minor comments. Looks great! |
652536c
to
f8b1c4b
Compare
f2ad0db
to
7fcbff7
Compare
f8b1c4b
to
cb44f1f
Compare
7fcbff7
to
cbbd4d3
Compare
…ubclasses. Previously, the Validation closure only exposed the optional request and response parameters making it difficult to customize validations in inline closures that were not custom extensions in the `Request` class. You didn’t have access to the actual response data. This was limiting in certain cases where you want to create a custom validation error that includes the error message buried in the server data. The only way you could do this previously was to write an extension on `Request`. Now you can do it in an inline closure right at the callsight. Another limitation was that the `Validation` closure did not expose the `destinationURL` for a `DownloadRequest` incase you needed to inspect the data in some special scenario. There is now a custom `Validation` closure on `DownloadRequest` that exposes the `destinationURL` directly. This allows you to inspect the file right in an inline closure without having to extension `DownloadRequest` directly.
cbbd4d3
to
722b701
Compare
Thanks for the review @jshier...much appreciated! 🍻 All comments have been addressed on this PR. I'm going to go ahead and move it through. |
This PR refactors the download destination system as well as the top-level APIs for constructing requests.
Motivation
Up until now, the
DownloadFileDestination
closures have been too limiting. They "force" you to move the file from the temporary URL to some other URL. Even if you don't want to move it, you still have to implement a destination closure and return the temp URL.A bigger issue is that you have to do all your preparation of the file system up front. You cannot modify the file system at the time the destination closure is called. For example, let's say I have an avatar image at
$Sandbox/Documents/avatar.jpg
and I want to update it. Right now, you have to either delete theavatar.jpg
file before making the request or move it to a different location. Otherwise you'll get a file system error when trying to move the file because a file already exists at the destination url. But you don't want to delete it before you make the request because you don't know that the request will actually succeed.On a different note, we've wanted to remove the
with
portion of thewithMethod
external parameter name in the top-level APIs. We've also wanted to figure out how add default values for the method without introducing ambiguity.Solution
Download File Destination Closure
The first major change in this PR is to make the
destination
closure optional in all the top-level APIs. You no longer have to move the file if you don't want to. It's now perfectly acceptable to do the following:If you do this, the file will stay in the temporary location. If you want to move it somewhere else, you'll need to handle that on your own.
Now if you do want to move the file to a different destination, you'll need to use the
DownloadFileDestination
closure which is a bit different.You'll notice that the typealias is the same as it was before with the exception of the
DownloadOptions
in the return type.Download Options
The
DownloadOptions
struct is anOptionSet
that comes with a couple cool new features..createIntermediateDirectories
- creates all intermediate directories to the destination url if specified.removePreviousFile
- removes a previous file at the destination url if it exists right before moving the temp file to the destination urlThese options allow you to customize the file system behavior right before the temp file is moved to the final destination url. Here's an example of what it looks like to use the new destination closure system.
DownloadResponse
The
DownloadResponse
has been modified to support both thetemporaryURL
anddestinationURL
properties. ThetemporaryURL
will always be set if the file was successfully downloaded. ThedestinationURL
will be set if the file was successfully downloaded and thedestination
closure was set. Make sure to check whether theResult
was a success though to know the file was able to be copied to thedestinationURL
. If the move operation fails, an error is generated, butdestinationURL
is notnil
. I designed it this way so that when an error occurs, you have as much information as possible to debug what happened.Download Request Validation
The
Validation
typealias for aDownloadRequest
has been modified to support both thetemporaryURL
anddestinationURL
.Top-Level APIs
The goal of the top-level API changes was to set
method
defaults and get rid of thewith
portion of thewithMethod
external parameter name. The results are pretty awesome. You can now do the following:Since this is certainly the majority of the use cases, this makes the call sights much more concise. If you do need to specify the
method
, it's now justmethod
instead ofwithMethod
.In order to disambiguate the
request
APIs, we had to add the externalresource
parameter name to theURLRequestConvertible
variant.The reason for this is that
URLRequest
conforms to bothURLStringConvertible
andURLRequestConvertible
. Rather than going down the route of removing conforms, I decided to just remove the potential of ambiguity altogether. Otherwise users could add their own extensions to types that would re-introduce ambiguity. And as well all know, ambiguous errors suck to debug.One thing that I think is still open is whether we add the
resource
name to the upload APIs.Thoughts here?
Summary
These changes greatly improve the functionality and usefulness of download requests by allowing them to support all the potential use cases. In the event that the
DownloadOptions
don't offer you enough flexibility, you can alway fall back to moving the file on your own in the response serializer. The top-level API changes are a huge improvement and make the common usage cases very concise.