Navigation Menu

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

Jump to bottom

AFResult #2774

Merged
merged 5 commits into from Mar 29, 2019
Merged

AFResult #2774

merged 5 commits into from Mar 29, 2019

Conversation

cnoon
Copy link
Member

@cnoon cnoon commented Mar 28, 2019

This PR is a combination of #2769 from @AtomicCat and the changes in db8b231 from @ejensen with changes from myself with what should be exposed.

Issue Link 🔗

#2752

Goals ⚽

There are a few:

  • Replace the Alamofire.Result type with the Foundation.Result type build by our own @jshier!
  • Remove any public extension APIs that duplicate the new Foundation.Result APIs.

Implementation Details 🚧

The main details of this implementation are described in-depth in #2752.

Testing Details 🔍

The test suite has been updated accordingly.

NOTE: I'll swing back here and add more detail to this PR shortly. have to run to a meeting

@cnoon cnoon added the result label Mar 28, 2019
@cnoon cnoon added this to the 5.0.0-beta.4 milestone Mar 28, 2019
@cnoon cnoon self-assigned this Mar 28, 2019
@cnoon cnoon requested a review from jshier March 28, 2019 20:59
@cnoon cnoon changed the title AFResult Typealias AFResult Mar 28, 2019
jshier
jshier requested changes Mar 28, 2019
View reviewed changes
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.

I think we need to be a bit more consistent about what we offer namespaced vs. not and public vs. not. If it's namespaced, it should be public, if we don't want it public, we should just make it part of the internal extension.

Source/Request.swift Show resolved Hide resolved
Source/AFResult.swift Outdated Show resolved Hide resolved
Source/Response.swift Outdated Show resolved Hide resolved
ejensen
ejensen reviewed Mar 28, 2019
View reviewed changes
Copy link
Contributor

@ejensen ejensen left a comment

Choose a reason for hiding this comment

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

A couple minor comments. Overall, lookin' good!

Source/AFResult.swift Outdated Show resolved Hide resolved
Tests/AFResultTests.swift Outdated Show resolved Hide resolved
jshier
jshier approved these changes Mar 29, 2019
View reviewed changes
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 some minor extension issues, but otherwise looks good to go!

Source/AFResult.swift Outdated Show resolved Hide resolved
Source/AFResult.swift Show resolved Hide resolved
Tests/AFResult+Alamofire.swift Outdated Show resolved Hide resolved
@cnoon cnoon mentioned this pull request Mar 29, 2019
Closed
@cnoon cnoon merged commit 38e19b1 into master Mar 29, 2019
@cnoon cnoon deleted the afresult-typealias branch March 29, 2019 00:29
@jshier jshier mentioned this pull request Mar 29, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jshier jshier jshier approved these changes

@AtomicCat AtomicCat AtomicCat left review comments

@ejensen ejensen ejensen left review comments

Assignees

@cnoon cnoon

Labels
result
Projects
None yet
Milestone
5.0.0-beta.4
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@cnoon @jshier @AtomicCat @ejensen @dave-kr