ronantakizawa/codereview-bench huggingface dataset:
Repository: vapor/vapor
File: Sources/Vapor/Droplet/Droplet.swift (Swift)

Code before:
    public var resourcesDir: String {
        return workDir + "Resources/"
    }

    /**
        The arguments passed to the droplet.
    */
    public let arguments: [String]

    /**
        The Database for this Droplet
        to run preparations on, if supplied.
    */
    public let database: Database?

    /**
        All of the providers supplied to the droplet.
    */
    public let providers: [Provider]

    /**
        The current droplet environment
    */
    public let environment: Environment

    /**
        Initialize the Droplet.
    */
    public init(
        // non-providable
        arguments: [String]? = nil,
        workDir workDirProvided: String? = nil,
        environment environmentProvided: Environment? = nil,
        config configProvided: Settings.Config? = nil,
        localization localizationProvided: Localization? = nil,

        // providable
        server: ServerProtocol.Type? = nil,
        hash: HashProtocol? = nil,
        cipher: CipherProtocol? = nil,
        console: ConsoleProtocol? = nil,
        log: Log? = nil,
        view: ViewRenderer? = nil,
        client: ClientProtocol.Type? = nil,
        database: Database? = nil,
        cache: CacheProtocol? = nil,

        // middleware
        availableMiddleware: [String: Middleware]? = nil,
        serverMiddleware: [String]? = nil,
        clientMiddleware: [String]? = nil,

Review comment (suggestion):
Could we have this as a computed property or something in another file? Looks a bit messy being here.


Code after:
    public var resourcesDir: String {
        return workDir + "Resources/"
    }

    /**
        The arguments passed to the droplet.
    */
    public let arguments: [String]

    /**
        The Database for this Droplet
        to run preparations on, if supplied.
    */
    public let database: Database?

    /**
        All of the providers supplied to the droplet.
    */
    public let providers: [Provider]

    /**
        The current droplet environment
    */
    public let environment: Environment

    internal private(set) lazy var routerResponder: Request.Handler = Request.Handler { [weak self] request in
        // Routed handler
        if let handler = self?.router.route(request, with: request) {
            return try handler.respond(to: request)
        } else {
            // Default not found handler
            let normal: [HTTP.Method] = [.get, .post, .put, .patch, .delete]

            if normal.contains(request.method) {
                throw Abort.notFound
            } else if case .options = request.method {
                return Response(status: .ok, headers: [
                    "Allow": "OPTIONS"
                    ])
            } else {
                return Response(status: .notImplemented)
            }
        }
    }



    /**
        Initialize the Droplet.
    */
    public init(
Repository: vapor/vapor
File: Sources/Vapor/Error/AbortMiddleware.swift (Swift)

Code before:
    public init() { }

    /**
        Respond to a given request chaining to the next

        - parameter request: request to process
        - parameter chain: next responder to pass request to

        - throws: an error on failure

        - returns: a valid response
     */
    public func respond(to request: Request, chainingTo chain: Responder) throws -> Response {
        do {
            return try chain.respond(to: request)
        } catch let error as AbortError {
            return try AbortMiddleware.errorResponse(request, error)
        }
    }

    public static func errorResponse(_ request: Request, _ error: AbortError) throws -> Response {
        if request.accept.prefers("html") {
            return ErrorView.shared.makeResponse(error.status, error.message)
        }

        let json = try JSON(node: [
            "error": true,
            "message": "\(error.message)",
            "code": error.code,
            "metadata": error.metadata
            ])
        let data = try json.makeBytes()
        let response = Response(status: error.status, body: .data(data))
        response.headers["Content-Type"] = "application/json; charset=utf-8"
        return response
    }
}



Review comment (suggestion):
can't change this API, let's keep original and then add this signature as a new function that calls the other internally

Code after:
    public init() { }

    /**
        Respond to a given request chaining to the next

        - parameter request: request to process
        - parameter chain: next responder to pass request to

        - throws: an error on failure

        - returns: a valid response
     */
    public func respond(to request: Request, chainingTo chain: Responder) throws -> Response {
        do {
            return try chain.respond(to: request)
        } catch let error as AbortError {
            return try AbortMiddleware.errorResponse(request, error)
        }
    }
    
    public static func errorResponse(_ request: Request, _ status: Status, _ message: String) throws -> Response {
        let error = Abort.custom(status: status, message: message)
        return try errorResponse(request, error)
    }
    
    public static func errorResponse(_ request: Request, _ error: AbortError) throws -> Response {
        if request.accept.prefers("html") {
            return ErrorView.shared.makeResponse(error.status, error.message)
        }

        let json = try JSON(node: [
            "error": true,
            "message": "\(error.message)",
            "code": error.code,
            "metadata": error.metadata
            ])
        let data = try json.makeBytes()
        let response = Response(status: error.status, body: .data(data))
        response.headers["Content-Type"] = "application/json; charset=utf-8"
        return response
    }
}
Repository: vapor/vapor
File: Tests/VaporTests/MiddlewareTests.swift (Swift)

Code before:
        XCTAssertEqual(res.headers["bar"], "baz")
    }

    func testMultiple() throws {
        let drop = try Droplet()

        drop.middleware = [
            FooMiddleware(),
            DateMiddleware()
        ]

        drop.get { _ in
            return "Hello, world"
        }

        let req = Request(method: .get, path: "/")
        let res = drop.respond(to: req)

        XCTAssert(res.headers["bar"] != nil)
        XCTAssert(res.headers["date"] != nil)
    }

    func testConfigClient() throws {
        let foo = FooMiddleware()

        let res = try EngineClientFactory().get("http://httpbin.org/headers", through: [foo])

        // test to make sure basic server saw the
        // header the middleware added
        XCTAssert(try res.bodyString().contains("Foo") == true)
        XCTAssert(try res.bodyString().contains("bar") == true)

        // test to make sure the middleware
        // added headers to the response
        XCTAssertEqual(res.headers["bar"], "baz")
    }

    func testConfigClientNotEnabled() throws {
        let drop = try Droplet()

        //drop.client.defaultMiddleware = []
        drop.middleware.append(FooMiddleware())

        let res = try drop.client.request(.get, "http://httpbin.org/headers")

        XCTAssert(try res.bodyString().contains("Foo") != true)
        XCTAssert(try res.bodyString().contains("bar") != true)
        XCTAssertNil(res.headers["bar"])
    }
}


Review comment (suggestion):
It would be nice if we could maintain the class level methods, or provide a default, I like `engineClient.get(` for most non-droplet situations

Code after:
        XCTAssertEqual(res.headers["bar"], "baz")
    }

    func testMultiple() throws {
        let drop = try Droplet()

        drop.middleware = [
            FooMiddleware(),
            DateMiddleware()
        ]

        drop.get { _ in
            return "Hello, world"
        }

        let req = Request(method: .get, path: "/")
        let res = drop.respond(to: req)

        XCTAssert(res.headers["bar"] != nil)
        XCTAssert(res.headers["date"] != nil)
    }

    func testConfigClient() throws {
        let foo = FooMiddleware()

        let res = try EngineClientFactory.shared.get("http://httpbin.org/headers", through: [foo])

        // test to make sure basic server saw the
        // header the middleware added
        XCTAssert(try res.bodyString().contains("Foo") == true)
        XCTAssert(try res.bodyString().contains("bar") == true)

        // test to make sure the middleware
        // added headers to the response
        XCTAssertEqual(res.headers["bar"], "baz")
    }

    func testConfigClientNotEnabled() throws {
        let drop = try Droplet()

        //drop.client.defaultMiddleware = []
        drop.middleware.append(FooMiddleware())

        let res = try drop.client.request(.get, "http://httpbin.org/headers")

        XCTAssert(try res.bodyString().contains("Foo") != true)
        XCTAssert(try res.bodyString().contains("bar") != true)
        XCTAssertNil(res.headers["bar"])
    }
}
Repository: vapor/vapor
File: Sources/Vapor/File/File+Response.swift (Swift)

Code before:
import Foundation
import HTTP

public extension Response {
    public convenience init(filePath: String, ifNoneMatch: String? = nil, chunkSize: Int = 2048) throws {
        guard
            let attributes = try? Foundation.FileManager.default.attributesOfItem(atPath: filePath),
            let modifiedAt = attributes[.modificationDate] as? Date,
            let fileSize = attributes[.size] as? NSNumber
            else {
                throw Abort.notFound
            }

        var headers: [HeaderKey: String] = [:]

        // Generate ETag value, "HEX value of last modified date" + "-" + "file size"
        let fileETag = "\(modifiedAt.timeIntervalSince1970)-\(fileSize.intValue)"
        headers["ETag"] = fileETag

        // Check if file has been cached already and return NotModified response if the etags match
        if fileETag == ifNoneMatch {
            self.init(status: .notModified, headers: headers, body: .data([]))
            return
        }

        // Set Content-Type header based on the media type
        // Only set Content-Type if file not modified and returned above.
        if
            let fileExtension = filePath.components(separatedBy: ".").last,
            let type = Request.mediaTypes[fileExtension]

Review comment (suggestion):
A comment here explaining what this does / maybe a link to the RFC for `If-None-Match` could be nice.

Code after:
import Foundation
import HTTP

public extension Response {
    /// If you are simply looking to serve files from your public directory,
    /// it may be useful to look at 'FileMiddleware' instead.
    ///
    /// Use this to initialize a file response for the exact file path.
    /// If using from a public folder for example, the file name should be appended
    /// to the public directory, ie: `drop.publicDir + "myFile.cool"`
    ///
    /// If none match represents an ETag that will be used to check if the file has
    /// changed since the last load by the client. This allows clients like browsers
    /// to cache their files and avoid downloading resources unnecessarily.
    /// Most often calculated w/
    /// https://tools.ietf.org/html/rfc7232#section-3.2
    ///
    /// For an example of how this is used, look at 'FileMiddleware'
    public convenience init(filePath: String, ifNoneMatch: String? = nil, chunkSize: Int = 2048) throws {
        guard
            let attributes = try? Foundation.FileManager.default.attributesOfItem(atPath: filePath),
            let modifiedAt = attributes[.modificationDate] as? Date,
            let fileSize = attributes[.size] as? NSNumber
            else {
                throw Abort.notFound
            }

        var headers: [HeaderKey: String] = [:]

        // Generate ETag value, "HEX value of last modified date" + "-" + "file size"
Repository: vapor/vapor
File: Sources/Logging/LogDestination.swift (Swift)

Code before:
public protocol LogDestination {
    /// Logs an encodable at the provided log level
    ///
    /// The encodable can be encoded to the required format
    ///
    /// The log level indicates the type of log and/or severity
    func log(_ encodable: Encodable, atLevel level: LogLevel, file: String, function: String, line: UInt, column: UInt)
}

extension LogDestination {
    /// Verbose logs are used to log tiny, usually irrelevant information.
    /// They are helpful when tracing specific lines of code and their results
    ///
    /// Example:
    ///
    ///     let a = 1 + 4
    ///     logger.debug(a)
    ///     let b = 1 - 3
    ///     logger.debug(b)
    ///     let c = (b + a) * 3
    ///     logger.debug(c)
    public func verbose(_ encodable: Encodable, file: String = #file, function: String = #function, line: UInt = #line, column: UInt = #column) {
        self.log(encodable, atLevel: .verbose, file: file, function: function, line: line, column: column)
    }
    
    /// Debug logs are used to debug problems
    ///
    /// Example:
    ///
    /// The password hash verification for user XYZ@example.com has failed/succeeded
    public func debug(_ encodable: Encodable, file: String = #file, function: String = #function, line: UInt = #line, column: UInt = #column) {
        self.log(encodable, atLevel: .debug, file: file, function: function, line: line, column: column)

Review comment (suggestion):
`LogLevel` is a strong type (not something generic like `Int` or `String`) so I don't think we need to compensate for any lacking type info by explicitly saying `atLevel`. I think this would be just as clear as more concise with `at: `. 

Code after:
public protocol LogDestination {
    /// Logs an encodable at the provided log level
    ///
    /// The encodable can be encoded to the required format
    ///
    /// The log level indicates the type of log and/or severity
    func log(_ encodable: Encodable, at level: LogLevel, file: String, function: String, line: UInt, column: UInt)
}

extension LogDestination {
    /// Verbose logs are used to log tiny, usually irrelevant information.
    /// They are helpful when tracing specific lines of code and their results
    ///
    /// Example:
    ///
    ///     let a = 1 + 4
    ///     logger.debug(a)
    ///     let b = 1 - 3
    ///     logger.debug(b)
    ///     let c = (b + a) * 3
    ///     logger.debug(c)
    public func verbose(_ encodable: Encodable, file: String = #file, function: String = #function, line: UInt = #line, column: UInt = #column) {
        self.log(encodable, at: .verbose, file: file, function: function, line: line, column: column)
    }
    
    /// Debug logs are used to debug problems
    ///
    /// Example:
    ///
    /// The password hash verification for user XYZ@example.com has failed/succeeded
    public func debug(_ encodable: Encodable, file: String = #file, function: String = #function, line: UInt = #line, column: UInt = #column) {
        self.log(encodable, at: .debug, file: file, function: function, line: line, column: column)
Repository: vapor/vapor
File: Sources/Routing/Route.swift (Swift)

Code before:
import HTTP
import Core

open class Route : Extendable {
    public var path: [PathComponent]
    public var method: Method
    public var responder: Responder
    
    public var extend = Extend()
    
    public init(method: Method, path: [PathComponent], responder: Responder) {
        self.method = method
        self.path = path
        self.responder = responder
    }
}


Review comment (suggestion):
Need comments. I'd also prefer doing a `final` class here and achieving the desired effect through protocols.

Code after:
import HTTP
import Core

/// A route. When registered to a router, replies to `Request`s using the `Responder`.
public final class Route: Extendable {
    /// The path at which the route is assigned
    public var path: [PathComponent]
    
    /// The method that this route responds to
    public var method: Method
    
    /// The responder. Used to respond to a `Request`
    public var responder: Responder
    
    /// A storage place to extend the `Route` with.
    /// Can store metadata like Documentation/Route descriptions
    public var extend = Extend()
    
    /// Creates a new route from a Method, path and responder
    public init(method: Method, path: [PathComponent], responder: Responder) {
        self.method = method
        self.path = path
        self.responder = responder
    }
}
Repository: vapor/vapor
File: Sources/Vapor/RoutingGroups.swift (Swift)

Code before:
import HTTP
import Routing

public final class Group : Router {
    let `super`: Router
    let components: [PathComponent]
    
    init(cascadingTo router: Router, components: [PathComponent] = []) {
        self.super = router
        self.components = components
    }
    
    public func register(route: Route) {
        route.path.insert(contentsOf: self.components, at: 0)
        self.super.register(route: route)
    }
    
    public func route(request: Request, parameters: inout ParameterBag) -> Responder? {
        return self.super.route(request: request, parameters: &parameters)
    }
}

extension Router {
    public func group(_ path: PathComponentRepresentable..., use: ((Group) -> ())) {
        use(Group(cascadingTo: self, components: path.makePathComponents()))
    }
    
    public func grouped(_ path: PathComponentRepresentable...) -> Group {
        return Group(cascadingTo: self)

Review comment (suggestion):
colon should be up against the type name (to follow rest of vapor style)

Code after:
import HTTP
import Routing

/// Groups routes
///
/// Every route will have the properties of this Group added
///
/// All path components will be inserted before the Route's path
///
/// All middleware will be applied to the Responder
public final class Group : Router {
    /// All routes registered to this group
    public private(set) var routes: [Route] = []
    
    let `super`: Router
    let components: [PathComponent]
    let middleware: [Middleware]
    
    /// Creates a new group
    ///
    /// All path components will be inserted before the Route's path
    ///
    /// All middleware will be applied to the Responder
    public init(cascadingTo router: Router, components: [PathComponent] = [], middleware: [Middleware] = []) {
        self.super = router
        self.components = components
        self.middleware = middleware
    }
Repository: vapor/vapor
File: Sources/Routing/TrieRouter.swift (Swift)

Code before:
import HTTP

/// A basic router
public final class TrieRouter: Router {
    /// The root node
    var root: RootNode

    public init() {
        self.root = RootNode()
    }

    /// See Router.register()
    public func register(route: Route) {
        // always start at the root node
        var current: TrieRouterNode = root

        let path = [.constant(route.method.string)] + route.path
        
        // traverse the path components supplied
        var iterator = path.makeIterator()
        while let path = iterator.next() {
            switch path {
            case .constant(let s):
                // find the child node matching this constant
                if let node = current.findConstantNode(at: s) {
                    current = node
                } else {
                    // if no child node matches this constant,
                    // we must create a new one
                    let new = ConstantNode(constant: s)
                    current.constantChildren.append(new)
                    current = new
                }
            case .parameter(let p):
                // there can only ever be one parameter node at
                // a given node in the tree, so always create a new one
                let new = ParameterNode(parameter: p)
                current.parameterChild = new
                current = new
            }
        }

Review comment (suggestion):
we should hold this route in an array (so that we can print details about them out later, or generate swagger, etc.)

Code after:
import HTTP

/// A basic router
public final class TrieRouter: Router {
    /// All routes registered to this router
    public private(set) var routes: [Route] = []
    
    /// The root node
    var root: RootNode

    public init() {
        self.root = RootNode()
    }

    /// See Router.register()
    public func register(route: Route) {
        self.routes.append(route)
        
        // always start at the root node
        var current: TrieRouterNode = root

        let path = [.constant(route.method.string)] + route.path
        
        // traverse the path components supplied
        var iterator = path.makeIterator()
        while let path = iterator.next() {
            switch path {
            case .constant(let s):
                // find the child node matching this constant
                if let node = current.findConstantNode(at: s) {
                    current = node
                } else {
                    // if no child node matches this constant,
                    // we must create a new one
                    let new = ConstantNode(constant: s)
                    current.constantChildren.append(new)
                    current = new
                }
            case .parameter(let p):
                // there can only ever be one parameter node at
                // a given node in the tree, so always create a new one
Repository: vapor/vapor
File: Sources/TCP/Socket/Socket.swift (Swift)

Code before:
import Core
import Dispatch
import libc

/// Any TCP socket. It doesn't specify being a server or client yet.
public class Socket {
    /// The file descriptor related to this socket
    public let descriptor: Descriptor

    /// The remote's address
    public var address: sockaddr_storage?
    
    /// True if the socket is non blocking
    public let isNonBlocking: Bool

    /// True if the socket should re-use addresses
    public let shouldReuseAddress: Bool
    
    /// Will be triggered before closing the socket, as part of the cleanup process
    public var beforeClose: (()->())? = nil

    /// Creates a TCP socket around an existing descriptor
    public init(
        established: Descriptor,
        isNonBlocking: Bool,
        shouldReuseAddress: Bool
    ) {
        self.descriptor = established
        self.isNonBlocking = isNonBlocking
        self.shouldReuseAddress = shouldReuseAddress
    }
    
    /// Creates a new TCP socket
    public convenience init(
        isNonBlocking: Bool = true,
        shouldReuseAddress: Bool = true

Review comment (suggestion):
I think we should wrap `sockaddr_storage` in a Swift struct. 

Code after:
import Bits
import Async
import Dispatch
import libc

/// Any TCP socket. It doesn't specify being a server or client yet.
public final class Socket {
    /// The file descriptor related to this socket
    public let descriptor: Int32

    /// The remote's address
    public var address: Address?
    
    /// True if the socket is non blocking
    public let isNonBlocking: Bool

    /// True if the socket should re-use addresses
    public let shouldReuseAddress: Bool
    
    /// A handler that gets called when closing the socket
    public typealias CloseHandler = (()->())
    
    /// Will be triggered before closing the socket, as part of the cleanup process
    public var didClose: CloseHandler? = nil
    
    /// A write source that's used to check when the connection is open
    internal var writeSource: DispatchSourceWrite?

    /// Creates a TCP socket around an existing descriptor
    public init(
        established: Int32,
        isNonBlocking: Bool,
        shouldReuseAddress: Bool,
        address: Address?
    ) {
        self.descriptor = established