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

Code before:
        var commandInput = CommandInput(arguments: arguments)
        return try Environment.detect(from: &commandInput)
    }
    
    /// Detects the environment from `CommandInput`. Parses the `--env` flag.
    /// - parameters:
    ///     - arguments: `CommandInput` to parse `--env` flag from.
    /// - returns: The detected environment, or default env.
    public static func detect(from commandInput: inout CommandInput) throws -> Environment {
        var env: Environment
        if let value = try commandInput.parse(option: .value(name: "env", short: "e")) {
            switch value {
            case "prod", "production": env = .production
            case "dev", "development": env = .development
            case "test", "testing": env = .testing
            default: env = .init(name: value)
            }
        } else {
            env = .development
        }
        env.commandInput = commandInput
        return env
    }
}

#warning("TODO: consider removing isRelease from environment, only rely on swift build mode")


Review comment (suggestion):
Not sure why this comment was removed in this PR, but I am against removing `isRelease`, if you have a staging and prod version of your app, it maybe should not behave as release on both staging and prod, but both will be compiled as swift production build mode

Code after:
extension Environment {
    /// Exposes the `Environment`'s `arguments` property as a `CommandInput`.
    public var commandInput: CommandInput {
        get { return CommandInput(arguments: arguments) }
        set { arguments = newValue.executablePath + newValue.arguments }
    }
    
    /// Detects the environment from `CommandLine.arguments`. Invokes `detect(from:)`.
    /// - parameters:
    ///     - arguments: Command line arguments to detect environment from.
    /// - returns: The detected environment, or default env.
    public static func detect(arguments: [String] = CommandLine.arguments) throws -> Environment {
        var commandInput = CommandInput(arguments: arguments)
        return try Environment.detect(from: &commandInput)
    }
    
    /// Detects the environment from `CommandInput`. Parses the `--env` flag.
    /// - parameters:
    ///     - arguments: `CommandInput` to parse `--env` flag from.
    /// - returns: The detected environment, or default env.
    public static func detect(from commandInput: inout CommandInput) throws -> Environment {
        var env: Environment
        if let value = try commandInput.parse(option: .value(name: "env", short: "e")) {
            switch value {
            case "prod", "production": env = .production
            case "dev", "development": env = .development
            case "test", "testing": env = .testing
            default: env = .init(name: value)
            }
        } else {
            env = .development
        }
        env.commandInput = commandInput
        return env
    }
}
Repository: vapor/vapor
File: Sources/XCTVapor/XCTApplication.swift (Swift)

Code before:
extension Application {
    public func xctest() -> XCTApplication {
        return .init(app: self)
    }
}


public final class XCTApplication {
    public let app: Application
    private var c: Container
    public init(app: Application) {
        self.app = app
        self.c = try! app.makeContainer().wait()
    }
    
    @discardableResult
    public func assert(
        _ method: HTTPMethod,
        to string: String,
        file: StaticString = #file,
        line: UInt = #line,
        closure: (XCTHTTPResponse) throws -> ()
    ) throws -> XCTApplication {
        let res = try self.c.make(Responder.self).respond(
            to: .init(method: .GET, urlString: string),
            using: .init(channel: EmbeddedChannel())
            ).wait()

Review comment (suggestion):
I'd name this `testable`.

```swift
self.app.testable()
    // ...
```
```suggestion
    public func testable() -> XCTApplication {
```

Code after:
extension Application {
    public func xctest() -> XCTApplication {
        return .init(app: self)
    }
}


public final class XCTApplication {
    public let app: Application
    private var serviceOverrides: [(inout Services) -> ()]
    
    private var _container: Container?
    public init(app: Application) {
        self.app = app
        self.serviceOverrides = []
    }
    
    @discardableResult
    public func override<T>(service: T.Type, with value: T) -> XCTApplication {
        return self.override(service: T.self) { _ in value }
    }
    
    public func override<T>(service: T.Type, with factory: @escaping (Container) throws -> T) -> XCTApplication {
        self.serviceOverrides.append({ services in
            services.register(T.self, factory)
        })
        return self
Repository: vapor/vapor
File: Sources/XCTVapor/XCTApplication.swift (Swift)

Code before:
extension Application {
    public func xctest() -> XCTApplication {
        return .init(app: self)
    }
}


public final class XCTApplication {
    public let app: Application
    private var c: Container
    public init(app: Application) {
        self.app = app
        self.c = try! app.makeContainer().wait()
    }
    
    @discardableResult
    public func assert(
        _ method: HTTPMethod,
        to string: String,
        file: StaticString = #file,
        line: UInt = #line,
        closure: (XCTHTTPResponse) throws -> ()
    ) throws -> XCTApplication {
        let res = try self.c.make(Responder.self).respond(
            to: .init(method: .GET, urlString: string),
            using: .init(channel: EmbeddedChannel())
            ).wait()
        try closure(.init(response: res))
        return self
    }
    
    deinit {
        try! self.c.shutdown().wait()
    }
}


Review comment (suggestion):
This should use the `method` argument to the function :)

Code after:
extension Application {
    public func xctest() -> XCTApplication {
        return .init(app: self)
    }
}


public final class XCTApplication {
    public let app: Application
    private var serviceOverrides: [(inout Services) -> ()]
    
    private var _container: Container?
    public init(app: Application) {
        self.app = app
        self.serviceOverrides = []
    }
    
    @discardableResult
    public func override<T>(service: T.Type, with value: T) -> XCTApplication {
        return self.override(service: T.self) { _ in value }
    }
    
    public func override<T>(service: T.Type, with factory: @escaping (Container) throws -> T) -> XCTApplication {
        self.serviceOverrides.append({ services in
            services.register(T.self, factory)
        })
        return self
    }
    
    @discardableResult
    public func test(
        _ method: HTTPMethod,
        to string: String,
        file: StaticString = #file,
        line: UInt = #line,
        closure: (XCTHTTPResponse) throws -> () = { _ in }
    ) throws -> XCTApplication {
        let res = try self.container().make(Responder.self).respond(
            to: .init(method: method, urlString: string),
            using: .init(channel: EmbeddedChannel())
        ).wait()
        try closure(.init(response: res))
        return self
    }
    
    private func container() throws -> Container {
        if let existing = self._container {
            return existing
        } else {
            var services = try self.app._makeServices()
Repository: vapor/vapor
File: Sources/XCTVapor/XCTApplication.swift (Swift)

Code before:
extension Application {
    public func xctest() -> XCTApplication {
        return .init(app: self)
    }
}


public final class XCTApplication {
    public let app: Application
    private var c: Container
    public init(app: Application) {
        self.app = app
        self.c = try! app.makeContainer().wait()
    }
    
    @discardableResult
    public func assert(
        _ method: HTTPMethod,
        to string: String,
        file: StaticString = #file,
        line: UInt = #line,
        closure: (XCTHTTPResponse) throws -> ()
    ) throws -> XCTApplication {
        let res = try self.c.make(Responder.self).respond(
            to: .init(method: .GET, urlString: string),
            using: .init(channel: EmbeddedChannel())
            ).wait()
        try closure(.init(response: res))
        return self
    }
    
    deinit {
        try! self.c.shutdown().wait()
    }
}


Review comment (suggestion):
Is there any way to syntactic sugar this to remove the `.init()`? Not a big deal, but was just wondering :)

Code after:
    
    @discardableResult
    public func override<T>(service: T.Type, with value: T) -> XCTApplication {
        return self.override(service: T.self) { _ in value }
    }
    
    public func override<T>(service: T.Type, with factory: @escaping (Container) throws -> T) -> XCTApplication {
        self.serviceOverrides.append({ services in
            services.register(T.self, factory)
        })
        return self
    }
    
    @discardableResult
    public func test(
        _ method: HTTPMethod,
        to string: String,
        file: StaticString = #file,
        line: UInt = #line,
        closure: (XCTHTTPResponse) throws -> () = { _ in }
    ) throws -> XCTApplication {
        let res = try self.container().make(Responder.self).respond(
            to: .init(method: method, urlString: string),
            using: .init(channel: EmbeddedChannel())
        ).wait()
        try closure(.init(response: res))
        return self
    }
    
    private func container() throws -> Container {
        if let existing = self._container {
            return existing
        } else {
            var services = try self.app._makeServices()
            for override in self.serviceOverrides {
                override(&services)
            }
            let new = try Container.boot(
                env: app.env,
                services: services,
                on: app.eventLoopGroup.next()
                ).wait()
            self._container = new
            return new
        }
    }
    
    deinit {
        try! self._container?.shutdown().wait()
    }
}
Repository: vapor/vapor
File: Tests/VaporTests/ApplicationTests.swift (Swift)

Code before:
import XCTVapor

extension Application {
    static func build(
        configure: @escaping (inout Services) throws -> () = { _ in },
        routes: @escaping (inout Routes, Container) throws -> () = { _, _ in }
    ) -> Application {
        return Application(env: .testing) {
            var s = Services.default()
            try configure(&s)
            s.extend(Routes.self) { r, c in
                try routes(&r, c)
            }
            return s
        }
    }
}

class ApplicationTests: XCTestCase {
    func testApplicationStop() throws {
        let test = Environment(name: "testing", arguments: ["vapor"])
        let app = Application(env: test) { .default() }
        app.eventLoopGroup.next().scheduleTask(in: .seconds(1)) {
            app.running?.stop()
        }
        try app.execute().wait()
    }
    
    

Review comment (suggestion):
Name this `create`?

```suggestion
    static func create(
```

Code after:
import XCTVapor

extension Application {
    static func create(
        configure: @escaping (inout Services) throws -> () = { _ in },
        routes: @escaping (inout Routes, Container) throws -> () = { _, _ in }
    ) -> Application {
        return Application(env: .testing) {
            var s = Services.default()
            try configure(&s)
            s.extend(Routes.self) { r, c in
                try routes(&r, c)
            }
            return s
        }
    }
}

class ApplicationTests: XCTestCase {
    func testApplicationStop() throws {
        let test = Environment(name: "testing", arguments: ["vapor"])
        let app = Application(env: test) { .default() }
        app.eventLoopGroup.next().scheduleTask(in: .seconds(1)) {
            app.running?.stop()
        }
        try app.execute().wait()
    }
Repository: vapor/vapor
File: Sources/Vapor/HTTP/ApplicationResponder.swift (Swift)

Code before:
public struct ApplicationResponder: Responder {
    private let router: Router
    
    private let requestsCountLabel: String = "http_requests_total"
    private let requestsTimerLabel: String = "http_request_duration_seconds"
    private let requestsErrorsLabel: String = "http_request_errors_total"

    init(_ router: Router) {
        self.router = router
    }
    
    public func respond(to request: Request) -> EventLoopFuture<Response> {
        let start = DispatchTime.now().uptimeNanoseconds
        let res = router.getRoute(for: request).responder.respond(to: request)
        
        res.whenComplete { result in
            guard let route = request.route else { return }
            switch result {
            case .success(let res):
                self.updateMetrics(for: request, on: route, label: self.requestsCountLabel, start: start, status: res.status.code)
            case .failure(_):
                self.updateMetrics(for: request, on: route, label: self.requestsErrorsLabel, start: start)
            }
        }
        
        return res

Review comment (suggestion):
Could we combine `ApplicationResponder` into `Router`? The separation seems kind of arbitrary atm. 

Code after:
extension Application {
    public var responder: Responder {
        ApplicationResponder(
            routes: self.routes,
            middleware: self.middleware.resolve()
        )
    }
}

/// Vapor's main `Responder` type. Combines configured middleware + router to create a responder.
internal struct ApplicationResponder: Responder {
    private let router: TrieRouter<Route>
    private let notFoundRoute: Route

    private let requestsCountLabel: String = "http_requests_total"
    private let requestsTimerLabel: String = "http_request_duration_seconds"
    private let requestsErrorsLabel: String = "http_request_errors_total"

    /// Creates a new `ApplicationResponder`
    public init(routes: Routes, middleware: [Middleware] = []) {
        // We create & store this at init time to not impact performance later on in the application.
        // This `Route` is used to return a 404 response, instead of an error.
        let notFoundResponder = middleware.makeResponder(chainingTo: BasicResponder(closure: { _ in throw Abort(.notFound) }))
        self.notFoundRoute = Route(method: .GET, path: [], responder: notFoundResponder, requestType: Request.self, responseType: Response.self)
        let router = TrieRouter(Route.self)
        for route in routes.all {
Repository: vapor/vapor
File: Sources/Vapor/HTTP/HTTPHeaderExpires.swift (Swift)

Code before:
import NIO

public struct HTTPHeaderExpires {
    /// The date represented by the header.
    public let expires: Date

    init?(dateString: String) {
        // https://tools.ietf.org/html/rfc7231#section-7.1.1.1
        let fmt = DateFormatter()
        fmt.locale = Locale(identifier: "en_US_POSIX")
        fmt.timeZone = TimeZone(secondsFromGMT: 0)
        fmt.dateFormat = "EEE, dd MMM yyyy hh:mm:ss zzz"
        
        if let date = fmt.date(from: dateString) {
            expires = date
            return
        }

        // Obsolete RFC 850 format
        fmt.dateFormat = "EEEE, dd-MMM-yy hh:mm:ss zzz"
        if let date = fmt.date(from: dateString) {
            expires = date
            return
        }

        // Obsolete ANSI C asctime() format
        fmt.dateFormat = "EEE MMM d hh:mm:s yyyy"
        if let date = fmt.date(from: dateString) {

Review comment (suggestion):
It could be nice to nest this under `HTTPHeaders` as `HTTPHeaders.Expire`. 

Code after:
import NIO

extension HTTPHeaders {
    public struct Expires {
        /// The date represented by the header.
        public let expires: Date

        internal static func parse(_ dateString: String) -> Expires? {
            // https://tools.ietf.org/html/rfc7231#section-7.1.1.1
            let fmt = DateFormatter()
            fmt.locale = Locale(identifier: "en_US_POSIX")
            fmt.timeZone = TimeZone(secondsFromGMT: 0)
            fmt.dateFormat = "EEE, dd MMM yyyy hh:mm:ss zzz"

            if let date = fmt.date(from: dateString) {
                return .init(expires: date)
            }

            // Obsolete RFC 850 format
            fmt.dateFormat = "EEEE, dd-MMM-yy hh:mm:ss zzz"
            if let date = fmt.date(from: dateString) {
                return .init(expires: date)
            }

            // Obsolete ANSI C asctime() format
            fmt.dateFormat = "EEE MMM d hh:mm:s yyyy"
            if let date = fmt.date(from: dateString) {
                return .init(expires: date)
Repository: vapor/vapor
File: Sources/Vapor/HTTP/HTTPHeaders+Cache.swift (Swift)

Code before:
extension HTTPHeaders {
    /// Gets the value of the `Cache-Control` header, if present.
    public func getCacheControl() -> HTTPHeaderCacheControl? {
        guard let cacheControl = firstValue(name: .cacheControl) else {
            return nil
        }

        return HTTPHeaderCacheControl(stringLiteral: cacheControl)
    }

    /// Gets the value of the `Expires` header, if present.
    /// ### Note ###
    /// `Expires` is legacy and you should switch to using `Cache-Control` if possible.
    public func getExpires() -> HTTPHeaderExpires? {
        HTTPHeaderExpires(headers: self)
    }

    /// Determines when the cached data should be expired.
    ///
    /// This first checks to see if the `Cache-Control` header is present.  If it is, and `no-store`
    /// is set, then `nil` is returned.  If `no-store` is not present, and there is a `max-age` then
    /// the expiration will add that many seconds to `requestSentAt`.
    ///
    /// If no `Cache-Control` header is present then it will examine the `Expires` header.
    ///
    /// - Parameter requestSentAt: Should be passed the `Date` when the request was sent.
    public func getExpirationDate(requestSentAt: Date) -> Date? {
        // Cache-Control header takes priority over the Expires header

Review comment (security):
The pattern for type-safe `HTTPHeaders` extensions elsewhere in the project is to do a mutable variable with the same name as the header key. That single variable then exposes all of the helper methods for working with the header value. For example:

```swift
var headers = HTTPHeaders(...)
headers.setCookie["foo"] = "bar"
headers.bearerAuthorization?.token
headers.contentType = .plaintext
```

These two new headers should be:
```swift
headers.cacheControl
headers.expires
```

Code after:
extension HTTPHeaders {
    /// Determines when the cached data should be expired.
    ///
    /// This first checks to see if the `Cache-Control` header is present.  If it is, and `no-store`
    /// is set, then `nil` is returned.  If `no-store` is not present, and there is a `max-age` then
    /// the expiration will add that many seconds to `requestSentAt`.
    ///
    /// If no `Cache-Control` header is present then it will examine the `Expires` header.
    ///
    /// If you need finer grained details about what type of caching, validation, etc... you should instead
    /// grab the `cacheControl` and `expires` headers yourself.
    ///
    /// - Parameter requestSentAt: Should be passed the `Date` when the request was sent.
    public func expirationDate(requestSentAt: Date) -> Date? {
        // Cache-Control header takes priority over the Expires header
        if let cacheControl = cacheControl {
            guard cacheControl.noStore == false else {
                return nil
            }

            if let age = cacheControl.maxAge {
                return requestSentAt.addingTimeInterval(TimeInterval(age))
            }
        }

        if let expires = expires {
            return expires.expires
        }
Repository: vapor/vapor
File: Sources/Vapor/HTTP/HTTPHeaderExpires.swift (Swift)

Code before:
        }

        // Obsolete ANSI C asctime() format
        fmt.dateFormat = "EEE MMM d hh:mm:s yyyy"
        if let date = fmt.date(from: dateString) {
            expires = date
            return
        }

        return nil
    }

    init?(headers: HTTPHeaders) {
        guard let str = headers.firstValue(name: .expires) else {
            return nil
        }

        self.init(dateString: str)
    }

    init(expires: Date) {
        self.expires = expires
    }

    /// Generates the header string for this instance.
    public func toString() -> String {
        let fmt = DateFormatter()
        fmt.locale = Locale(identifier: "en_US_POSIX")
        fmt.timeZone = TimeZone(secondsFromGMT: 0)
        fmt.dateFormat = "EEE, dd MMM yyyy hh:mm:ss zzz"

        return fmt.string(from: expires)
    }
}


Review comment (suggestion):
Same comment here regarding using `CustomStringConvertible` or a `serialize` / `parse` method combo.

Code after:

            // Obsolete ANSI C asctime() format
            fmt.dateFormat = "EEE MMM d hh:mm:s yyyy"
            if let date = fmt.date(from: dateString) {
                return .init(expires: date)
            }

            return nil
        }

        init(expires: Date) {
            self.expires = expires
        }

        /// Generates the header string for this instance.
        public func serialize() -> String {
            let fmt = DateFormatter()
            fmt.locale = Locale(identifier: "en_US_POSIX")
            fmt.timeZone = TimeZone(secondsFromGMT: 0)
            fmt.dateFormat = "EEE, dd MMM yyyy hh:mm:ss zzz"

            return fmt.string(from: expires)
        }
    }

    /// Gets the value of the `Expires` header, if present.
    /// ### Note ###
    /// `Expires` is legacy and you should switch to using `CacheControl` if possible.
    public var expires: Expires? {
        get { firstValue(name: .expires).flatMap(Expires.parse) }
        set {
            if let new = newValue?.serialize() {
                self.replaceOrAdd(name: .expires, value: new)
            } else {
                self.remove(name: .expires)
            }
        }
    }
}
Repository: vapor/vapor
File: Tests/VaporTests/LoggingTests.swift (Swift)

Code before:
import Vapor
import XCTest

class LoggingTests : XCTestCase {

    func testNotFoundLogging() throws {
        
        var services = Services.default()
        var config = Config.default()
        let loggerProvider = TestLoggerProvider()
                
        try services.register(loggerProvider)
        config.prefer(TestLogger.self, for: Logger.self)
        
        let app = try Application(config: config, services: services)

        let req = Request(
            http: HTTPRequest(method: .GET, url: "/hello/vapor"),
            using: app
        )
        
        do {
            _ = try app.make(Responder.self).respond(to: req).wait()
        } catch {}
        
        XCTAssert(loggerProvider.logger.didLog(string: "Abort.404: /hello/vapor Not Found"))
    }
    
    func testInternalServerErrorLogging() throws {

Review comment (suggestion):
This new file needs to be included in the LinuxMain file. Or this could just be added to an existing file like `ApplicationTests.swift`.

Code after:
import Vapor
import XCTest

class LoggingTests : XCTestCase {

    func testNotFoundLogging() throws {
        
        var services = Services.default()
        var config = Config.default()
        let loggerProvider = TestLoggerProvider()
                
        try services.register(loggerProvider)
        config.prefer(TestLogger.self, for: Logger.self)
        
        let app = try Application(config: config, services: services)

        let req = Request(
            http: HTTPRequest(method: .GET, url: "/hello/vapor"),
            using: app
        )
        
        do {
            _ = try app.make(Responder.self).respond(to: req).wait()
        } catch {}
        
        XCTAssert(loggerProvider.logger.didLog(string: "Abort.404: GET /hello/vapor Not Found"))
    }
    
    func testInternalServerErrorLogging() throws {