ronantakizawa/codereview-bench huggingface dataset:
Repository: Moya/Moya
File: Moya+ReactiveCocoa.swift (Swift)

Code before:
//
//  Moya+ReactiveCocoa.swift
//  Moya
//
//  Created by Ash Furrow on 2014-08-22.
//  Copyright (c) 2014 Ash Furrow. All rights reserved.
//

import Foundation

/// Subclass of MoyaProvider that returns RACSignal instances when requests are made. Much better than using completion closures.
public class ReactiveMoyaProvider<T where T: MoyaTarget>: MoyaProvider<T> {
    /// Current requests that have not completed or errored yet.
    /// Note: Do not access this directly. It is public only for unit-testing purposes (sigh).
    public var inflightRequests = Dictionary<Endpoint<T>, RACSignal>()
    
    /// Initializes a reactive provider.
    override public init(endpointsClosure: MoyaEndpointsClosure, endpointResolver: MoyaEndpointResolution = MoyaProvider.DefaultEnpointResolution(), stubResponses: Bool = false) {
        super.init(endpointsClosure: endpointsClosure, endpointResolver: endpointResolver, stubResponses: stubResponses)
    }
    
    /// Designated request-making method.
    public func request(token: T, method: Moya.Method, parameters: [String: AnyObject]) -> RACSignal {
        let endpoint = self.endpoint(token, method: method, parameters: parameters)
        
        if let existingSignal = inflightRequests[endpoint] {
            return existingSignal
        }
        
        // weak self just for best practices – RACSignal will take care of any retain cycles anyway,
        // and we're connecting immediately (below), so self in the block will always be non-nil
        let signal = RACSignal.createSignal({ [weak self] (subscriber) -> RACDisposable! in
            self?.request(token, method: method, parameters: parameters) { (data: NSData?, error: NSError?) -> () in
                if let error = error {
                    subscriber.sendError(error)
                } else {

Review comment (suggestion):
I definitely wanted to make it a struct, but I couldn't send it on `RACSubject` since it "does not conform to AnyObject protocol" :(


Code after:
//
//  Moya+ReactiveCocoa.swift
//  Moya
//
//  Created by Ash Furrow on 2014-08-22.
//  Copyright (c) 2014 Ash Furrow. All rights reserved.
//

import Foundation

public class MoyaResponse {
    public let statusCode: Int
    public let data: NSData
    
    public init(statusCode: Int, data: NSData) {
        self.statusCode = statusCode
        self.data = data
    }
}

extension MoyaResponse: Printable, DebugPrintable {
    public var description: String {
        return "Status Code: \(statusCode), Data Length: \(data.length)"
    }
    
    public var debugDescription: String {
        return description
    }
}

/// Subclass of MoyaProvider that returns RACSignal instances when requests are made. Much better than using completion closures.
public class ReactiveMoyaProvider<T where T: MoyaTarget>: MoyaProvider<T> {
    /// Current requests that have not completed or errored yet.
    /// Note: Do not access this directly. It is public only for unit-testing purposes (sigh).
    public var inflightRequests = Dictionary<Endpoint<T>, RACSignal>()
Repository: Moya/Moya
File: Source/ReactiveCore/MoyaError.swift (Swift)

Code before:
import Foundation

public let MoyaErrorDomain = "Moya"

public enum MoyaErrorCode: Int {
    case ImageMapping = 0
    case JSONMapping
    case StringMapping
    case StatusCode
    case Data
}

public enum MoyaError: ErrorType {
    case ImageMapping(MoyaResponse)
    case JSONMapping(MoyaResponse)
    case StringMapping(MoyaResponse)
    case StatusCode(Int,MoyaResponse)
    case Data(MoyaResponse)
    case Underlying(ErrorType)
    
    public func sameErrorType(otherError: MoyaError) -> Bool {
        switch self {
        case .ImageMapping:
            switch otherError {
            case .ImageMapping:
                return true
            default:
                return false
            }
        case .JSONMapping:
            switch otherError {
            case .JSONMapping:
                return true
            default:
                return false
            }
        case .StringMapping:
            switch otherError {
            case .StringMapping:
                return true
            default:
                return false
            }
        case .StatusCode:
            switch otherError {
            case .StatusCode:
                return true
            default:
                return false
            }
        case .Data:
            switch otherError {
            case .Data:
                return true
            default:
                return false
            }
        case .Underlying:
            switch otherError {
            case .Underlying:
                return true
            default:
                return false
            }
        }
    }
}

Review comment (suggestion):
Making the error conform to Equatable would be a better solution in my opinion. 


Code after:
import Foundation

public let MoyaErrorDomain = "Moya"

public enum MoyaErrorCode: Int {
    case ImageMapping = 0
    case JSONMapping
    case StringMapping
    case StatusCode
    case Data
}

public enum MoyaError: ErrorType {
    case ImageMapping(MoyaResponse)
    case JSONMapping(MoyaResponse)
    case StringMapping(MoyaResponse)
    case StatusCode(MoyaResponse)
    case Data(MoyaResponse)
    case Underlying(ErrorType)
}
Repository: Moya/Moya
File: Source/ReactiveCore/MoyaError.swift (Swift)

Code before:
import Foundation

public let MoyaErrorDomain = "Moya"

public enum MoyaErrorCode: Int {
    case ImageMapping = 0
    case JSONMapping
    case StringMapping
    case StatusCode
    case Data
}

public enum MoyaError: ErrorType {
    case ImageMapping(MoyaResponse)
    case JSONMapping(MoyaResponse)
    case StringMapping(MoyaResponse)
    case StatusCode(Int,MoyaResponse)
    case Data(MoyaResponse)
    case Underlying(ErrorType)
    
    public func sameErrorType(otherError: MoyaError) -> Bool {
        switch self {
        case .ImageMapping:
            switch otherError {
            case .ImageMapping:
                return true
            default:
                return false
            }
        case .JSONMapping:
            switch otherError {
            case .JSONMapping:
                return true
            default:
                return false
            }
        case .StringMapping:
            switch otherError {
            case .StringMapping:
                return true
            default:
                return false
            }
        case .StatusCode:
            switch otherError {
            case .StatusCode:
                return true
            default:
                return false
            }
        case .Data:
            switch otherError {
            case .Data:
                return true
            default:
                return false
            }
        case .Underlying:
            switch otherError {
            case .Underlying:
                return true
            default:
                return false
            }
        }
    }
}

Review comment (suggestion):
Agreed, this function works great. Moving it to the test target sounds great too, we can always move it back later. 


Code after:
import Foundation

public let MoyaErrorDomain = "Moya"

public enum MoyaErrorCode: Int {
    case ImageMapping = 0
    case JSONMapping
    case StringMapping
    case StatusCode
    case Data
}

public enum MoyaError: ErrorType {
    case ImageMapping(MoyaResponse)
    case JSONMapping(MoyaResponse)
    case StringMapping(MoyaResponse)
    case StatusCode(MoyaResponse)
    case Data(MoyaResponse)
    case Underlying(ErrorType)
}
Repository: Moya/Moya
File: Source/Moya.swift (Swift)

Code before:
import Result

/// Closure to be executed when a request has completed.
public typealias Completion = (result: Result<Moya.Response, Moya.Error>) -> ()

/// Represents an HTTP method.
public enum Method: String {
    case GET, POST, PUT, DELETE, OPTIONS, HEAD, PATCH, TRACE, CONNECT
}

public enum StubBehavior {
    case Never
    case Immediate
    case Delayed(seconds: NSTimeInterval)
}

/// Protocol to define the base URL, path, method, parameters and sample data for a target.
public protocol TargetType {
    var baseURL: NSURL { get }
    var path: String { get }
    var method: Moya.Method { get }
    var parameters: [String: AnyObject]? { get }
    var sampleData: NSData { get }
}

public enum StructTarget<T: TargetType>: TargetType {
    case Struct(T)

    public init(_ target: T) {
        self = StructTarget.Struct(target)
    }

    public var path: String {
        switch self {
        case .Struct(let t): return t.path
        }
    }

    public var baseURL: NSURL {
        switch self {
        case .Struct(let t): return t.baseURL
        }
    }

    public var method: Moya.Method {
        switch self {
        case .Struct(let t): return t.method
        }
    }

    public var parameters: [String: AnyObject]? {

Review comment (suggestion):
Nice one! I really like that 🙌 One thing I can add to this is that we can create a computed property for example that will give us the target, e.g.:

``` swift
private var target: T {
    switch self {
        case .Struct(let t): return t
    }
}
```

and then we can skip the switch stuff in other properties with just e.g. `return target.path`.


Code after:
import Result

/// Closure to be executed when a request has completed.
public typealias Completion = (result: Result<Moya.Response, Moya.Error>) -> ()

/// Represents an HTTP method.
public enum Method: String {
    case GET, POST, PUT, DELETE, OPTIONS, HEAD, PATCH, TRACE, CONNECT
}

public enum StubBehavior {
    case Never
    case Immediate
    case Delayed(seconds: NSTimeInterval)
}

/// Protocol to define the base URL, path, method, parameters and sample data for a target.
public protocol TargetType {
    var baseURL: NSURL { get }
    var path: String { get }
    var method: Moya.Method { get }
    var parameters: [String: AnyObject]? { get }
    var sampleData: NSData { get }
}

public enum StructTarget<T: TargetType>: TargetType {
    case Struct(T)

    public init(_ target: T) {
        self = StructTarget.Struct(target)
    }

    public var path: String {
        return target.path
    }

    public var baseURL: NSURL {
        return target.baseURL
    }

    public var method: Moya.Method {
        return target.method
    }

    public var parameters: [String: AnyObject]? {
        return target.parameters
    }

    public var sampleData: NSData {
        return target.sampleData
    }
Repository: Moya/Moya
File: Sources/Moya/Moya+Alamofire.swift (Swift)

Code before:

    init(request: Request) {
        self.request = request
        self.cancelAction = {
            request.cancel()
        }
    }

    public var debugDescription: String {
        guard let request = self.request else {
            return "Empty Request"
        }
        return request.debugDescription
    }

}

internal typealias RequestableCompletion = (HTTPURLResponse?, URLRequest?, Data?, Swift.Error?) -> Void

internal protocol Requestable {
    func response(queue: DispatchQueue?, completionHandler: @escaping RequestableCompletion) -> Self
}

extension DataRequest: Requestable {
    internal func response(queue: DispatchQueue?, completionHandler: @escaping RequestableCompletion) -> Self {
        return response(queue: queue) { handler  in
            completionHandler(handler.response, handler.request, handler.data, handler.error)
        }
    }
}

extension DownloadRequest: Requestable {
    internal func response(queue: DispatchQueue?, completionHandler: @escaping RequestableCompletion) -> Self {
        return response(queue: queue) { handler  in
            completionHandler(handler.response, handler.request, nil, handler.error)
        }
    }
}


Review comment (suggestion):
This argument label remains as `queue` because it's a function from Alamofire :wink:

Code after:

    init(request: Request) {
        self.request = request
        self.cancelAction = {
            request.cancel()
        }
    }

    public var debugDescription: String {
        guard let request = self.request else {
            return "Empty Request"
        }
        return request.debugDescription
    }

}

internal typealias RequestableCompletion = (HTTPURLResponse?, URLRequest?, Data?, Swift.Error?) -> Void

internal protocol Requestable {
    func response(callbackQueue: DispatchQueue?, completionHandler: @escaping RequestableCompletion) -> Self
}

extension DataRequest: Requestable {
    internal func response(callbackQueue: DispatchQueue?, completionHandler: @escaping RequestableCompletion) -> Self {
        return response(queue: callbackQueue) { handler  in
            completionHandler(handler.response, handler.request, handler.data, handler.error)
        }
    }
}

extension DownloadRequest: Requestable {
    internal func response(callbackQueue: DispatchQueue?, completionHandler: @escaping RequestableCompletion) -> Self {
        return response(queue: callbackQueue) { handler  in
            completionHandler(handler.response, handler.request, nil, handler.error)
        }
    }
}
Repository: Moya/Moya
File: Sources/Moya/Moya+Alamofire.swift (Swift)

Code before:
    public var debugDescription: String {
        guard let request = self.request else {
            return "Empty Request"
        }
        return request.debugDescription
    }

}

internal typealias RequestableCompletion = (HTTPURLResponse?, URLRequest?, Data?, Swift.Error?) -> Void

internal protocol Requestable {
    func response(queue: DispatchQueue?, completionHandler: @escaping RequestableCompletion) -> Self
}

extension DataRequest: Requestable {
    internal func response(queue: DispatchQueue?, completionHandler: @escaping RequestableCompletion) -> Self {
        return response(queue: queue) { handler  in
            completionHandler(handler.response, handler.request, handler.data, handler.error)
        }
    }
}

extension DownloadRequest: Requestable {
    internal func response(queue: DispatchQueue?, completionHandler: @escaping RequestableCompletion) -> Self {
        return response(queue: queue) { handler  in
            completionHandler(handler.response, handler.request, nil, handler.error)
        }
    }
}


Review comment (suggestion):
This argument label remains as `queue` because it's a function from Alamofire :wink:

Code after:
    public var debugDescription: String {
        guard let request = self.request else {
            return "Empty Request"
        }
        return request.debugDescription
    }

}

internal typealias RequestableCompletion = (HTTPURLResponse?, URLRequest?, Data?, Swift.Error?) -> Void

internal protocol Requestable {
    func response(callbackQueue: DispatchQueue?, completionHandler: @escaping RequestableCompletion) -> Self
}

extension DataRequest: Requestable {
    internal func response(callbackQueue: DispatchQueue?, completionHandler: @escaping RequestableCompletion) -> Self {
        return response(queue: callbackQueue) { handler  in
            completionHandler(handler.response, handler.request, handler.data, handler.error)
        }
    }
}

extension DownloadRequest: Requestable {
    internal func response(callbackQueue: DispatchQueue?, completionHandler: @escaping RequestableCompletion) -> Self {
        return response(queue: callbackQueue) { handler  in
            completionHandler(handler.response, handler.request, nil, handler.error)
        }
    }
}
Repository: Moya/Moya
File: Sources/Moya/Plugins/AccessTokenPlugin.swift (Swift)

Code before:
import Foundation
import Result

// MARK: - AccessTokenAuthorizable

/// A protocol for controlling the behavior of `AccessTokenPlugin`.
public protocol AccessTokenAuthorizable {

    /// Represents the authorization header to use for requests.
    var authorizationType: AuthorizationType { get }
}

// MARK: - AuthorizationType

/// An enum representing the header to use with an `AccessTokenPlugin`
public enum AuthorizationType: String {
    case none
    case basic = "Basic "
    case bearer = "Bearer "
}

// MARK: - AccessTokenPlugin

/**
 A plugin for adding basic or bearer-type authorization headers to requests. Example:

 ```
 Authorization: Bearer <token>
 Authorization: Basic <token>
 ```

*/
public struct AccessTokenPlugin: PluginType {

    /// A closure returning the access token to be applied in the header.
    public let tokenClosure: () -> String

    /**
     Initialize a new `AccessTokenPlugin`.

     - parameters:
       - tokenClosure: A closure returning the token to be applied in the pattern `Authorization: <AuthorizationType> <token>`
    */

Review comment (security):
The trailing space feels a bit weird. I'm assuming it's because of the formatting in:
```swift 
request.addValue(authorizationType.rawValue + tokenClosure(), forHTTPHeaderField: "Authorization")
```

If so, maybe we take the space out of the enum and properly combine `AuthorizationType` with the `tokenClosure` when adding the header?

Code after:
import Foundation
import Result

// MARK: - AccessTokenAuthorizable

/// A protocol for controlling the behavior of `AccessTokenPlugin`.
public protocol AccessTokenAuthorizable {

    /// Represents the authorization header to use for requests.
    var authorizationType: AuthorizationType { get }
}

// MARK: - AuthorizationType

/// An enum representing the header to use with an `AccessTokenPlugin`
public enum AuthorizationType: String {
    case none
    case basic = "Basic"
    case bearer = "Bearer"
}

// MARK: - AccessTokenPlugin

/**
 A plugin for adding basic or bearer-type authorization headers to requests. Example:

 ```
 Authorization: Bearer <token>
 Authorization: Basic <token>
 ```

*/
public struct AccessTokenPlugin: PluginType {

    /// A closure returning the access token to be applied in the header.
    public let tokenClosure: () -> String

    /**
     Initialize a new `AccessTokenPlugin`.

     - parameters:
       - tokenClosure: A closure returning the token to be applied in the pattern `Authorization: <AuthorizationType> <token>`
    */
Repository: Moya/Moya
File: Tests/TestHelpers.swift (Swift)

Code before:
import Moya

#if os(OSX)
import AppKit
#else
import UIKit
import Foundation
#endif

// MARK: - Mock Services
enum GitHub {
    case zen
    case userProfile(String)
}

extension GitHub: TargetType {
    var baseURL: URL { return URL(string: "https://api.github.com")! }
    var path: String {
        switch self {
        case .zen:
            return "/zen"
        case .userProfile(let name):
            return "/users/\(name.urlEscaped)"
        }
    }

    var method: Moya.Method {
        return .get

Review comment (suggestion):
We may want to stick to more verbose conditional statements (as we had before):
```
  #if os(iOS) || os(watchOS) || os(tvOS)
    ...
  #elseif os(OSX)
    ...
  #endif
```

Code after:
import Moya


#if os(iOS) || os(watchOS) || os(tvOS)
import UIKit
import Foundation
#elseif os(OSX)
import AppKit
#endif

// MARK: - Mock Services
enum GitHub {
    case zen
    case userProfile(String)
}

extension GitHub: TargetType {
    var baseURL: URL { return URL(string: "https://api.github.com")! }
    var path: String {
        switch self {
        case .zen:
            return "/zen"
        case .userProfile(let name):
            return "/users/\(name.urlEscaped)"
        }
    }

    var method: Moya.Method {
Repository: Moya/Moya
File: Sources/Moya/URLRequest+Encoding.swift (Swift)

Code before:
import Foundation

internal extension URLRequest {

    mutating func encoded(encodable: Encodable) throws -> URLRequest {
        do {
            let encodable = AnyEncodable(encodable)
            httpBody = try JSONEncoder().encode(encodable)

            if value(forHTTPHeaderField: "Content-Type") == nil {
                setValue("application/json", forHTTPHeaderField: "Content-Type")
            }

            return self
        } catch {
            throw MoyaError.encodableMapping(error)
        }
    }

    func encoded(parameters: [String: Any], parameterEncoding: ParameterEncoding) throws -> URLRequest {
        do {
            return try parameterEncoding.encode(self, with: parameters)
        } catch {
            throw MoyaError.parameterEncoding(error)
        }
    }
}


Review comment (suggestion):
Maybe we can add a local constant `let contentType = "Content-Type"` here and use that instead of typing it twice? To prevent regressions in the future.

Code after:
import Foundation

internal extension URLRequest {

    mutating func encoded(encodable: Encodable) throws -> URLRequest {
        do {
            let encodable = AnyEncodable(encodable)
            httpBody = try JSONEncoder().encode(encodable)

            let contentTypeHeaderName = "Content-Type"
            if value(forHTTPHeaderField: contentTypeHeaderName) == nil {
                setValue("application/json", forHTTPHeaderField: contentTypeHeaderName)
            }

            return self
        } catch {
            throw MoyaError.encodableMapping(error)
        }
    }

    func encoded(parameters: [String: Any], parameterEncoding: ParameterEncoding) throws -> URLRequest {
        do {
            return try parameterEncoding.encode(self, with: parameters)
        } catch {
            throw MoyaError.parameterEncoding(error)
        }
    }
}
Repository: Moya/Moya
File: Sources/Moya/MoyaProvider+Internal.swift (Swift)

Code before:
import Foundation
import Result

// MARK: - Method

extension Method {
    /// A Boolean value determining whether the request supports multipart.
    /// True: .post, .put, .patch, .connect
    /// False: .get, .delete, .head, .options, .trace
    public var supportsMultipart: Bool {
        switch self {
        case .post, .put, .patch, .connect:
            return true
        case .get, .delete, .head, .options, .trace:
            return false
        }
    }
}

// MARK: - MoyaProvider

/// Internal extension to keep the inner-workings outside the main Moya.swift file.
public extension MoyaProvider {
    /// Performs normal requests.
    func requestNormal(_ target: Target, callbackQueue: DispatchQueue?, progress: Moya.ProgressBlock?, completion: @escaping Moya.Completion) -> Cancellable {
        let endpoint = self.endpoint(target)
        let stubBehavior = self.stubClosure(target)
        let cancellableToken = CancellableWrapper()

        // Allow plugins to modify response
        let pluginsWithCompletion: Moya.Completion = { result in
            let processedResult = self.plugins.reduce(result) { $1.process($0, target: target) }
            completion(processedResult)

Review comment (suggestion):
I would avoid giving implementation details in the comment - once you change the method you would have to remember to change the comment too. First line should be sufficient.

Code after:
import Foundation
import Result

// MARK: - Method

extension Method {
    /// A Boolean value determining whether the request supports multipart.
    public var supportsMultipart: Bool {
        switch self {
        case .post, .put, .patch, .connect:
            return true
        case .get, .delete, .head, .options, .trace:
            return false
        }
    }
}

// MARK: - MoyaProvider

/// Internal extension to keep the inner-workings outside the main Moya.swift file.
public extension MoyaProvider {
    /// Performs normal requests.
    func requestNormal(_ target: Target, callbackQueue: DispatchQueue?, progress: Moya.ProgressBlock?, completion: @escaping Moya.Completion) -> Cancellable {
        let endpoint = self.endpoint(target)
        let stubBehavior = self.stubClosure(target)
        let cancellableToken = CancellableWrapper()

        // Allow plugins to modify response
        let pluginsWithCompletion: Moya.Completion = { result in
            let processedResult = self.plugins.reduce(result) { $1.process($0, target: target) }
            completion(processedResult)
        }