Skip to content
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

Replace all internal references to AFResult<T> with Result<T, Error> #2891

Merged
merged 2 commits into from
Aug 9, 2019

Conversation

philtre
Copy link
Contributor

@philtre philtre commented Aug 7, 2019

Goals ⚽

Remove all internal references to AFResult but keep the AFResult typealias for migration purposes.

This is an attempt to break apart #2864

Copy link
Contributor

@jshier jshier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few style changes, and a I think you can keep AFResult and the Result extensions in one file. Whether you want to just move AFResult or undo the separation of files doesn't matter to me, but they don't need to be separate.

@@ -25,83 +25,3 @@
/// `Result` that always has an `Error` `Failure` type.
public typealias AFResult<T> = Result<T, Error>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's really no reason to have just this typealias in its own file. It can be combined with the Result extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved the typealias to Result+Alamofire.swift

@@ -189,7 +189,7 @@ extension DataRequest {
appendResponseSerializer {
// Start work that should be on the serialization queue.
let start = CFAbsoluteTimeGetCurrent()
let result = AFResult { try responseSerializer.serialize(request: self.request,
let result = Result<Serializer.SerializedObject, Error> { try responseSerializer.serialize(request: self.request,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please adjust the parameter alignment, and perhaps put the try on its own line to it isn't as wide.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@jshier jshier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks for the work here!

Anything to add @cnoon?

Copy link
Member

@cnoon cnoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me @philtre. Thank you for putting this together! Keeping the AFResult typealias is wise for those out there that have lots of code built on the betas. 👍🏻

@jshier jshier merged commit 03433df into Alamofire:master Aug 9, 2019
@jshier jshier added this to the 5.0.0.rc.1 milestone Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants