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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify and refactor #2691

Closed
wants to merge 16 commits into from

Conversation

Projects
None yet
3 participants
@jdisho
Copy link
Contributor

commented Jan 15, 2019

This is my first PR. First of all, I want to thank you for your time investment in Alamofire. Amazing lib 馃憦

Goals 鈿斤笍

Simplifying and refactoring some basic things.

Implementation Details 馃毀

  • Most of which are simplification of switch statements. The switch cases which are simplified are 2 case statements and can we simplified by using guard.
  • I set the default value of queue to .main rather than nil. I did this because under the hood, .main is used when the queue value is nil.
  • I remove methodDependent property in ParameterEncoding since it is a duplication of default. (If there is a reason why methodDependent and default are included, I would be interested to know why 馃槃)
  • I simplified the encodesParametersInURL in ParameterEncoding by making it part of Destination. (I occurred that in the same way encodesParametersInURL is implemented in ParameterEncoder)

jdisho added some commits Jan 15, 2019

@jdisho jdisho changed the title Minor/refactor Simplify and refactor Jan 15, 2019

@jshier
Copy link
Contributor

left a comment

Thanks for the PR! Some of the changes go against some intentional design decisions but I think some of the other can be merged with changes.

Show resolved Hide resolved Source/AFError.swift
Show resolved Hide resolved Source/ParameterEncoder.swift Outdated
Show resolved Hide resolved Source/ParameterEncoding.swift Outdated
Show resolved Hide resolved Source/Result.swift Outdated
@@ -109,7 +109,7 @@ extension DataRequest {
/// - completionHandler: The code to be executed once the request has finished.
/// - Returns: The request.
@discardableResult
public func response(queue: DispatchQueue? = nil, completionHandler: @escaping (DataResponse<Data?>) -> Void) -> Self {
public func response(queue: DispatchQueue = .main, completionHandler: @escaping (DataResponse<Data?>) -> Void) -> Self {

This comment has been minimized.

Copy link
@jshier

jshier Jan 16, 2019

Contributor

If we're going to make this change, it would be good to audit all API that takes optional values with a coalesced value in the body, and change them all to work like this.

This comment has been minimized.

Copy link
@jdisho

jdisho Jan 16, 2019

Author Contributor

From what I saw, I couldn't find any similar case to this.

}
}
}

// MARK: Properties
/// Returns a default `URLEncoding` instance.
/// Returns a default `URLEncoding` instance with a `.methodDependent` destination.

This comment has been minimized.

Copy link
@jshier

jshier Jan 16, 2019

Contributor

I'm not sure about this one. Perhaps keeping default and adding an httpBody convenience property?

This comment has been minimized.

Copy link
@jdisho

jdisho Jan 16, 2019

Author Contributor

I am not understanding your comment. I need more context. What is the issue about?

@jdisho

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

Thank you for your response @jshier. I will try to address your comments as soon as possible.

@jshier
Copy link
Contributor

left a comment

I think we should reevaluate these changes after beta 2 is released, as there will likely need to be some updates.

@cnoon cnoon self-requested a review Mar 26, 2019

@cnoon cnoon self-assigned this Mar 26, 2019

@cnoon cnoon added this to the 5.0.0-beta.4 milestone Mar 26, 2019

@cnoon

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

Thanks for putting this all together @jdisho. I'm going to take a stab at putting together a different PR with most of these changes while keeping your attribution. I'll link the new PR once it's up.

@cnoon

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

Hi @jdisho,

I just pushed up #2765 which pulls in these changes, culls out a few of them, and rebases on top of the #2716 PR. I made sure to maintain your attribution to the changeset. I'm going to go ahead and close out this PR for now. Please redirect all further comments to #2765. These changes will go out as part of AF5 beta 4.

Cheers. 馃嵒

@cnoon cnoon closed this Mar 26, 2019

cnoon added a commit that referenced this pull request Mar 27, 2019

cnoon added a commit that referenced this pull request Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.