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

Code before:
        return self.secret(path: filePath, fileIO: fileIO, on: eventLoop)
    }


    /// Load the content of a file at a given path as a secret.
    ///
    /// - Parameters:
    ///   - path: Path to the file containing the secret
    ///   - fileIO: `NonBlockingFileIO` handler provided by NIO
    ///   - eventLoop: `EventLoop` for NIO to use for working with the file
    ///
    /// - Returns:
    ///   - On success, a succeeded future with the loaded content of the file.
    ///   - On any kind of error, a succeeded future with a value of `nil`. It is not currently possible to get error details.
    public static func secret(path: String, fileIO: NonBlockingFileIO, on eventLoop: EventLoop) -> EventLoopFuture<String?> {
        return fileIO
            .openFile(path: path, eventLoop: eventLoop)
            .flatMap { handle, region in
                return fileIO
                    .read(fileRegion: region, allocator: .init(), eventLoop: eventLoop)
                    .always { _ in try? handle.close() }
            }
            .map { buffer -> String in
                return buffer
                    .getString(at: buffer.readerIndex, length: buffer.readableBytes)!
                       .trimmingCharacters(in: .whitespacesAndNewlines)
            }
            .recover { _ -> String? in
                nil
            }
    }

}


Review comment (suggestion):
```suggestion
                return buffer
                    .getString(at: buffer.readerIndex, length: buffer.readableBytes)!
                    .trimmingCharacters(in: .whitespacesAndNewlines)
```

Code after:
    }


    /// Load the content of a file at a given path as a secret.
    ///
    /// - Parameters:
    ///   - path: Path to the file containing the secret
    ///   - fileIO: `NonBlockingFileIO` handler provided by NIO
    ///   - eventLoop: `EventLoop` for NIO to use for working with the file
    ///
    /// - Returns:
    ///   - On success, a succeeded future with the loaded content of the file.
    ///   - On any kind of error, a succeeded future with a value of `nil`. It is not currently possible to get error details.
    public static func secret(path: String, fileIO: NonBlockingFileIO, on eventLoop: EventLoop) -> EventLoopFuture<String?> {
        return fileIO
            .openFile(path: path, eventLoop: eventLoop)
            .flatMap { handle, region in
                return fileIO
                    .read(fileRegion: region, allocator: .init(), eventLoop: eventLoop)
                    .always { _ in try? handle.close() }
            }
            .map { buffer -> String in
                return buffer
                    .getString(at: buffer.readerIndex, length: buffer.readableBytes)!
                    .trimmingCharacters(in: .whitespacesAndNewlines)
            }
            .recover { _ -> String? in
                nil
            }
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Environment/Environment+Process.swift (Swift)

Code before:
extension Environment {    
    /// The process information of an environment. Wraps `ProcessInto.processInfo`.
    @dynamicMemberLookup public struct Process {

        /// The process information of the environment.
        private let _info: ProcessInfo
        
        /// Creates a new `Process` wrapper for process information.
        ///
        /// - parameter info: The process info that the wrapper accesses. Defaults to `ProcessInto.processInfo`.
        internal init(info: ProcessInfo = .processInfo) {
            self._info = info
        }
        
        /// Gets a variable's value from the process' environment, and converts it to generic type `T`.
        ///
        ///     Environment.process.DATABASE_PORT = 3306
        ///     Environment.process.DATABASE_PORT // 3306
        public subscript<T>(dynamicMember member: String) -> T? where T: LosslessStringConvertible {
            get {
                return self._info.environment[member].flatMap { T($0) }
            }

            nonmutating set (value) {
                if let raw = value?.description {
                    setenv(member, raw, 1)
                } else {
                    unsetenv(member)
                }
            }

Review comment (suggestion):
```suggestion
    @dynamicMemberLookup public struct Process {
        /// The process information of the environment.
```

Code after:
extension Environment {    
    /// The process information of an environment. Wraps `ProcessInto.processInfo`.
    @dynamicMemberLookup public struct Process {
        /// The process information of the environment.
        private let _info: ProcessInfo
        
        /// Creates a new `Process` wrapper for process information.
        ///
        /// - parameter info: The process info that the wrapper accesses. Defaults to `ProcessInto.processInfo`.
        internal init(info: ProcessInfo = .processInfo) {
            self._info = info
        }
        
        /// Gets a variable's value from the process' environment, and converts it to generic type `T`.
        ///
        ///     Environment.process.DATABASE_PORT = 3306
        ///     Environment.process.DATABASE_PORT // 3306
        public subscript<T>(dynamicMember member: String) -> T? where T: LosslessStringConvertible {
            get {
                return self._info.environment[member].flatMap { T($0) }
            }

            nonmutating set (value) {
                if let raw = value?.description {
                    setenv(member, raw, 1)
                } else {
                    unsetenv(member)
                }
            }
        }
Repository: vapor/vapor
File: Sources/Vapor/Authentication/RedirectMiddleware.swift (Swift)

Code before:
    ///
    /// - parameters:
    ///    - path: The path to redirect to if the request is not authenticated
    public static func redirectMiddleware(path: String) -> Middleware {
        return RedirectMiddleware<Self>(Self.self, path: path)
    }
    
    /// Basic middleware to redirect unauthenticated requests to the supplied path
    ///
    /// - parameters:
    ///    - pathClosure: The closure that returns the redirect path based on the given URI path
    ///    - input: The original `Request` object of the request
    public static func redirectMiddleware(pathClosure: @escaping (_ input: Request) -> String) -> Middleware {
        return RedirectMiddleware<Self>(Self.self, pathClosure: pathClosure)
    }
}


private final class RedirectMiddleware<A>: Middleware
    where A: Authenticatable
{
    let pathClosure: PathClosure
    
    typealias PathClosure = (Request) -> String

    init(_ authenticatableType: A.Type = A.self, path: String) {
        self.pathClosure = { _ in return path }
    }
    
    init(_ authenticatableType: A.Type = A.self, pathClosure: @escaping PathClosure) {
        self.pathClosure = pathClosure
    }

    /// See Middleware.respond
    func respond(to req: Request, chainingTo next: Responder) -> EventLoopFuture<Response> {
        if req.auth.has(A.self) {
            return next.respond(to: req)
        }
        
        let redirectPath = pathClosure(req)
        
        let redirect = req.redirect(to: redirectPath)
        return req.eventLoop.makeSucceededFuture(redirect)
    }
}


Review comment (suggestion):
This initializer is internal so we can delete it, no need for two. As long as we keep `public static func redirectMiddleware(path: String) -> Middleware` we're fine. 

Code after:
    ///
    /// - parameters:
    ///    - path: The path to redirect to if the request is not authenticated
    public static func redirectMiddleware(path: String) -> Middleware {
        self.redirectMiddleware(makePath: { _ in path })
    }
    
    /// Basic middleware to redirect unauthenticated requests to the supplied path
    ///
    /// - parameters:
    ///    - makePath: The closure that returns the redirect path based on the given `Request` object
    public static func redirectMiddleware(makePath: @escaping (Request) -> String) -> Middleware {
        RedirectMiddleware<Self>(Self.self, makePath: makePath)
    }
}


private final class RedirectMiddleware<A>: Middleware
    where A: Authenticatable
{
    let makePath: (Request) -> String
    
    init(_ authenticatableType: A.Type = A.self, makePath: @escaping (Request) -> String) {
        self.makePath = makePath
    }

    /// See Middleware.respond
    func respond(to req: Request, chainingTo next: Responder) -> EventLoopFuture<Response> {
        if req.auth.has(A.self) {
            return next.respond(to: req)
        }

        let redirect = req.redirect(to: self.makePath(req))
        return req.eventLoop.makeSucceededFuture(redirect)
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Authentication/RedirectMiddleware.swift (Swift)

Code before:
extension Authenticatable {
    /// Basic middleware to redirect unauthenticated requests to the supplied path
    ///
    /// - parameters:
    ///    - path: The path to redirect to if the request is not authenticated
    public static func redirectMiddleware(path: String) -> Middleware {
        return RedirectMiddleware<Self>(Self.self, path: path)
    }
    
    /// Basic middleware to redirect unauthenticated requests to the supplied path
    ///
    /// - parameters:
    ///    - pathClosure: The closure that returns the redirect path based on the given URI path
    ///    - input: The original `Request` object of the request
    public static func redirectMiddleware(pathClosure: @escaping (_ input: Request) -> String) -> Middleware {
        return RedirectMiddleware<Self>(Self.self, pathClosure: pathClosure)
    }
}


private final class RedirectMiddleware<A>: Middleware
    where A: Authenticatable
{
    let pathClosure: PathClosure
    
    typealias PathClosure = (Request) -> String

    init(_ authenticatableType: A.Type = A.self, path: String) {
        self.pathClosure = { _ in return path }
    }
    
    init(_ authenticatableType: A.Type = A.self, pathClosure: @escaping PathClosure) {
        self.pathClosure = pathClosure
    }

    /// See Middleware.respond
    func respond(to req: Request, chainingTo next: Responder) -> EventLoopFuture<Response> {
        if req.auth.has(A.self) {
            return next.respond(to: req)
        }

Review comment (suggestion):
I think `makePath`, `pathFactory`, or just `_` would be better names here. The closure definition doesn't need a label or doc block entry either, that can be just `(Request) -> String`. 

Code after:
extension Authenticatable {
    /// Basic middleware to redirect unauthenticated requests to the supplied path
    ///
    /// - parameters:
    ///    - path: The path to redirect to if the request is not authenticated
    public static func redirectMiddleware(path: String) -> Middleware {
        self.redirectMiddleware(makePath: { _ in path })
    }
    
    /// Basic middleware to redirect unauthenticated requests to the supplied path
    ///
    /// - parameters:
    ///    - makePath: The closure that returns the redirect path based on the given `Request` object
    public static func redirectMiddleware(makePath: @escaping (Request) -> String) -> Middleware {
        RedirectMiddleware<Self>(Self.self, makePath: makePath)
    }
}


private final class RedirectMiddleware<A>: Middleware
    where A: Authenticatable
{
    let makePath: (Request) -> String
    
    init(_ authenticatableType: A.Type = A.self, makePath: @escaping (Request) -> String) {
        self.makePath = makePath
    }

    /// See Middleware.respond
    func respond(to req: Request, chainingTo next: Responder) -> EventLoopFuture<Response> {
        if req.auth.has(A.self) {
            return next.respond(to: req)
        }

        let redirect = req.redirect(to: self.makePath(req))
        return req.eventLoop.makeSucceededFuture(redirect)
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Routing/Parameters+Require.swift (Swift)

Code before:
public extension Parameters {
    /// Grabs the named parameter from the parameter bag.
    /// An `Abort(.unprocessableEntity)` error is thrown if the parameter does not exist.
    func require(_ name: String, error: Error = Abort(.unprocessableEntity)) throws -> String {
        guard let value = get(name) else {
            throw error
        }

        return value
    }

    /// Grabs the named parameter from the parameter bag, casting it to
    /// a `LosslessStringConvertible` type.
    /// An `Abort(.unprocessableEntity)` error is thrown if the parameter does not exist.
    func require<T>(_ name: String, as type: T.Type = T.self, error: Error = Abort(.unprocessableEntity)) throws -> T?
        where T: LosslessStringConvertible
    {
        try T.init(require(name, error: error))
    }
}


Review comment (suggestion):
I'd prefer the label of the error to be `or`, so `or error: Error`.

This follows the style used by `EventLoopFuture.unwrap` seen here: https://github.com/vapor/vapor/blob/a6aa7f337ca76b9861273fc0a75f6d7cfe719ba6/Sources/Vapor/Utilities/OptionalType.swift#L11

Code after:
extension Parameters {
    /// Grabs the named parameter from the parameter bag.
    /// If the parameter does not exist, `Abort(.internalServerError)` is thrown.
    /// If the parameter value cannot be converted to `String`, `Abort(.unprocessableEntity)` is thrown.
    ///
    /// - parameters:
    ///     - name: The name of the parameter.
    public func require(_ name: String) throws -> String {
        return try self.require(name, as: String.self)
    }

    /// Grabs the named parameter from the parameter bag, casting it to a `LosslessStringConvertible` type.
    /// If the parameter does not exist, `Abort(.internalServerError)` is thrown.
    /// If the parameter value cannot be converted to the required type, `Abort(.unprocessableEntity)` is thrown.
    ///
    /// - parameters:
    ///     - name: The name of the parameter.
    ///     - type: The required parameter value type.
    public func require<T>(_ name: String, as type: T.Type = T.self) throws -> T
        where T: LosslessStringConvertible
    {
        guard let stringValue: String = get(name) else {
            throw Abort(.internalServerError)
        }

        guard let value = T.init(stringValue) else {
            throw Abort(.unprocessableEntity)
        }
Repository: vapor/vapor
File: Sources/Vapor/Routing/Parameters+Require.swift (Swift)

Code before:
public extension Parameters {
    /// Grabs the named parameter from the parameter bag.
    /// An `Abort(.unprocessableEntity)` error is thrown if the parameter does not exist.
    func require(_ name: String, or error: Error = Abort(.unprocessableEntity)) throws -> String {
        guard let value = get(name) else {
            throw error
        }

        return value
    }

    /// Grabs the named parameter from the parameter bag, casting it to
    /// a `LosslessStringConvertible` type.
    /// An `Abort(.unprocessableEntity)` error is thrown if the parameter does not exist.
    func require<T>(_ name: String, as type: T.Type = T.self, or error: Error = Abort(.unprocessableEntity)) throws -> T
        where T: LosslessStringConvertible
    {
        guard let value = try T.init(require(name, or: error)) else {
            throw error
        }

        return value
    }
}


Review comment (suggestion):
Please put `public` in front of each method instead of using the `public extension` syntax (Vapor style). 

Code after:
extension Parameters {
    /// Grabs the named parameter from the parameter bag.
    /// If the parameter does not exist, `Abort(.internalServerError)` is thrown.
    /// If the parameter value cannot be converted to `String`, `Abort(.unprocessableEntity)` is thrown.
    ///
    /// - parameters:
    ///     - name: The name of the parameter.
    public func require(_ name: String) throws -> String {
        return try self.require(name, as: String.self)
    }

    /// Grabs the named parameter from the parameter bag, casting it to a `LosslessStringConvertible` type.
    /// If the parameter does not exist, `Abort(.internalServerError)` is thrown.
    /// If the parameter value cannot be converted to the required type, `Abort(.unprocessableEntity)` is thrown.
    ///
    /// - parameters:
    ///     - name: The name of the parameter.
    ///     - type: The required parameter value type.
    public func require<T>(_ name: String, as type: T.Type = T.self) throws -> T
        where T: LosslessStringConvertible
    {
        guard let stringValue: String = get(name) else {
            throw Abort(.internalServerError)
        }

        guard let value = T.init(stringValue) else {
Repository: vapor/vapor
File: Sources/Vapor/Routing/Parameters+Require.swift (Swift)

Code before:
extension Parameters {
    /// Grabs the named parameter from the parameter bag.
    /// If the parameter does not exist, `Abort(.internalServerError)` is thrown.
    /// If the parameter value cannot be converted to `String`, `Abort(.unprocessableEntity)` is thrown.
    ///
    /// - parameters:
    ///     - name: The name of the parameter.
    public func require(_ name: String) throws -> String {
        return try self.require(name, as: String.self)
    }

    /// Grabs the named parameter from the parameter bag, casting it to a `LosslessStringConvertible` type.
    /// If the parameter does not exist, `Abort(.internalServerError)` is thrown.
    /// If the parameter value cannot be converted to the generic type, `Abort(.unprocessableEntity)` is thrown.
    ///
    /// - parameters:
    ///     - name: The name of the parameter.
    public func require<T>(_ name: String, as type: T.Type = T.self) throws -> T
        where T: LosslessStringConvertible
    {
        guard let stringValue: String = get(name) else {
            throw Abort(.internalServerError)
        }

        guard let value = T.init(stringValue) else {
            throw Abort(.unprocessableEntity)
        }

        return value
    }
}


Review comment (suggestion):
Would rename this to the required type

```suggestion
    /// If the parameter value cannot be converted to the required type, `Abort(.unprocessableEntity)` is thrown.
```

Code after:
extension Parameters {
    /// Grabs the named parameter from the parameter bag.
    /// If the parameter does not exist, `Abort(.internalServerError)` is thrown.
    /// If the parameter value cannot be converted to `String`, `Abort(.unprocessableEntity)` is thrown.
    ///
    /// - parameters:
    ///     - name: The name of the parameter.
    public func require(_ name: String) throws -> String {
        return try self.require(name, as: String.self)
    }

    /// Grabs the named parameter from the parameter bag, casting it to a `LosslessStringConvertible` type.
    /// If the parameter does not exist, `Abort(.internalServerError)` is thrown.
    /// If the parameter value cannot be converted to the required type, `Abort(.unprocessableEntity)` is thrown.
    ///
    /// - parameters:
    ///     - name: The name of the parameter.
    ///     - type: The required parameter value type.
    public func require<T>(_ name: String, as type: T.Type = T.self) throws -> T
        where T: LosslessStringConvertible
    {
        guard let stringValue: String = get(name) else {
            throw Abort(.internalServerError)
        }

        guard let value = T.init(stringValue) else {
            throw Abort(.unprocessableEntity)
        }

        return value
    }
}
Repository: vapor/vapor
File: Sources/XCTVapor/XCTApplication.swift (Swift)

Code before:
extension Application: XCTApplicationTester {
    public func performTest(request: XCTHTTPRequest) throws -> XCTHTTPResponse {
         try self.testable().performTest(request: request)
    }
}

extension Application {
    public enum Method {
        case inMemory
        case running(port: Int)
        case runningWith(hostname: String, port: Int)
        public static var running: Method {
            return .runningWith(hostname:"localhost", port: 8080)
        }
    }

    public func testable(method: Method = .inMemory) throws -> XCTApplicationTester {
        try self.boot()
        switch method {
        case .inMemory:
            return try InMemory(app: self)
        case .running(let port):
            return try Live(app: self, port: port)
        case let .runningWith(hostname, port):
            return try Live(app: self, hostname: hostname, port: port)
        }
    }
    
    private struct Live: XCTApplicationTester {
        let app: Application
        let port: Int
        let hostname: String

        init(app: Application, hostname: String = "localhost", port: Int) throws {
            self.app = app
            self.hostname = hostname

Review comment (suggestion):
Although this enum is public, no one should be switching on it. I think we're fine to add a case here.

Something like this would be cleaner:
```swift
case inMemory
static func running(port: Int) -> Self {
    .running(hostname: "localhost", port: port)
}
case running(hostname: String, port: Int)
```

Code after:
extension Application: XCTApplicationTester {
    public func performTest(request: XCTHTTPRequest) throws -> XCTHTTPResponse {
         try self.testable().performTest(request: request)
    }
}

extension Application {
    public enum Method {
        case inMemory
        public static var running: Method {
            return .running(hostname:"localhost", port: 8080)
        }
        public static func running(port: Int) -> Self {
            .running(hostname: "localhost", port: port)
        }
        case running(hostname: String, port: Int)
    }

    public func testable(method: Method = .inMemory) throws -> XCTApplicationTester {
        try self.boot()
        switch method {
        case .inMemory:
            return try InMemory(app: self)
        case let .running(hostname, port):
            return try Live(app: self, hostname: hostname, port: port)
        }
    }
    
    private struct Live: XCTApplicationTester {
        let app: Application
        let port: Int
        let hostname: String

        init(app: Application, hostname: String = "localhost", port: Int) throws {
            self.app = app
            self.hostname = hostname