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

Code before:

}

extension ValidatorResults {
    /// `ValidatorResult` of a validator thaat validates whether the data can be represented as a specific Enum case.
    public struct Case<T, E: RawRepresentable> where E.RawValue == T {
        /// The type of the enum to check.
        let enumType: E.Type
        /// The raw value that would be tested agains the enum type.
        let rawValue: T
        /// Helps Generate better failure description.
        let possibleCases: [CustomStringConvertible]?
    }
}

extension ValidatorResults.Case: ValidatorResult {
    public var isFailure: Bool {
        return enumType.init(rawValue: rawValue) == nil
    }

    public var successDescription: String? {
        "is \(E.self)"
    }

    public var failureDescription: String? {
        var message = "value {\(rawValue)} cannot be represented as \(E.self)."
        if let cases = self.possibleCases {
            message += " Possiblue values: \(cases.map { "\($0)" }.joined(separator: ", "))"
        }
        return message

    }
}


Review comment (bug):
The name of the enum seems like an implementation detail to me. This failure description should be similar to the `.in` validator and say something like "value is not foo, bar, or baz"

Code after:
        let rawValue: T
    }
}

extension ValidatorResults.Case: ValidatorResult {
    public var isFailure: Bool {
        return enumType.init(rawValue: rawValue) == nil
    }

    public var successDescription: String? {
        makeDescription(not: false)
    }

    public var failureDescription: String? {
        makeDescription(not: true)
    }

    func makeDescription(not: Bool) -> String {
        let items = E.allCases.map { "\($0.rawValue)" }
        let description: String
        switch items.count {
        case 1:
            description = items[0].description
        case 2:
            description = "\(items[0].description) or \(items[1].description)"
        default:
            let first = items[0..<(items.count - 1)]
                .map { $0.description }.joined(separator: ", ")
            let last = items[items.count - 1].description
            description = "\(first), or \(last)"
        }
        return "is\(not ? " not" : "") \(description)"
    }
}
Repository: vapor/vapor
File: Sources/Vapor/HTTP/EndpointCache.swift (Swift)

Code before:

/// Handles the complexities of HTTP caching.
open class EndpointCache<T: Decodable> {
    public enum CachingDataError: Swift.Error {
        case unexpctedResponseStatus(HTTPStatus, uri: URI)
        case badJSON(Error)
    }

    internal var cached: T?
    internal var request: EventLoopFuture<T>?
    internal var headers: HTTPHeaders?
    internal var cacheUntil: Date?

    private let sync: Lock
    private let uri: URI

    /// The designated initializer.
    /// - Parameters:
    ///   - uri: The `URI` of the resource to be downloaded.
    public init(uri: URI) {
        self.uri = uri
        self.sync = .init()
    }

    /// Downloads the resource.
    /// - Parameters:
    ///   - request: The `Request` which is initiating the download.
    ///   - logger: An optional logger
    public func get(on request: Request, logger: Logger? = nil) -> EventLoopFuture<T> {

Review comment (performance):
This should either be internal and named just `Error` or outside of the class so that it's not generic. Otherwise to catch it you need to do `EndpointCache<T>.CachingDataError`.

Code after:
public enum EndpointCacheError: Swift.Error {
    case unexpctedResponseStatus(HTTPStatus, uri: URI)
    case contentDecodeFailure(Error)
}

/// Handles the complexities of HTTP caching.
public final class EndpointCache<T> where T: Decodable {
    private var cached: T?
    private var request: EventLoopFuture<T>?
    private var headers: HTTPHeaders?
    private var cacheUntil: Date?

    private let sync: Lock
    private let uri: URI

    /// The designated initializer.
    /// - Parameters:
    ///   - uri: The `URI` of the resource to be downloaded.
    public init(uri: URI) {
        self.uri = uri
        self.sync = .init()
    }

    /// Downloads the resource.
    /// - Parameters:
    ///   - request: The `Request` which is initiating the download.
    ///   - logger: An optional logger
    public func get(on request: Request, logger: Logger? = nil) -> EventLoopFuture<T> {
        return self.download(on: request.eventLoop, using: request.client, logger: logger ?? request.logger)
Repository: vapor/vapor
File: Sources/Vapor/HTTP/EndpointCache.swift (Swift)

Code before:
public enum CachingDataError: Swift.Error {
    case unexpctedResponseStatus(HTTPStatus, uri: URI)
    case badJSON(Error)
}

/// Handles the complexities of HTTP caching.
public final class EndpointCache<T> where T: Decodable {
    private var cached: T?
    private var request: EventLoopFuture<T>?
    private var headers: HTTPHeaders?
    private var cacheUntil: Date?

    private let sync: Lock
    private let uri: URI

    /// The designated initializer.
    /// - Parameters:
    ///   - uri: The `URI` of the resource to be downloaded.
    public init(uri: URI) {
        self.uri = uri
        self.sync = .init()
    }

    /// Downloads the resource.
    /// - Parameters:
    ///   - request: The `Request` which is initiating the download.

Review comment (performance):
```suggestion
public enum EndpointCacheError: Swift.Error {
```

Code after:
public enum EndpointCacheError: Swift.Error {
    case unexpctedResponseStatus(HTTPStatus, uri: URI)
    case contentDecodeFailure(Error)
}

/// Handles the complexities of HTTP caching.
public final class EndpointCache<T> where T: Decodable {
    private var cached: T?
    private var request: EventLoopFuture<T>?
    private var headers: HTTPHeaders?
    private var cacheUntil: Date?

    private let sync: Lock
    private let uri: URI

    /// The designated initializer.
    /// - Parameters:
    ///   - uri: The `URI` of the resource to be downloaded.
    public init(uri: URI) {
        self.uri = uri
        self.sync = .init()
    }

    /// Downloads the resource.
    /// - Parameters:
    ///   - request: The `Request` which is initiating the download.
Repository: vapor/vapor
File: Sources/Vapor/Routing/RoutesBuilder+WebSocket.swift (Swift)

Code before:
struct WebSocketRouteConfiguration {
    let maxFrameSize: Int
    let onUpgrade: (WebSocket) -> ()
}

extension RoutesBuilder {
    @discardableResult
    public func webSocket(
        _ path: PathComponent...,
        maxFrameSize: Int = 1 << 14, // This default was copied from NIOWebSocketServerUpgrader's defaults
        onUpgrade: @escaping (Request, WebSocket) -> ()
    ) -> Route {
        return self.on(.GET, path) { request -> Response in
            let configuration = WebSocketRouteConfiguration(maxFrameSize: maxFrameSize, onUpgrade: { ws in
                onUpgrade(request, ws)
            })
            let res = Response(status: .switchingProtocols)
            res.upgrader = .webSocket(configuration: configuration)
            return res
        }
    }
}


Review comment (suggestion):
I'm not a fan of adding parameters here. Especially not considering more parameters can be added later on. 

@tanner0101 can we get away with returning a `WebSocketRoute` instance? 
Doing so would allow configuring the route's `WebSocketConfiguration` like so:

```swift
router.webSocket("path", "to", "ws") { websocket in
    ....
}.maxFrameSize(1_000_000)
```

Code after:
public enum WebSocketMaxFrameSize {
    case `default`
    case override(Int)
}

extension RoutesBuilder {
    @discardableResult
    public func webSocket(
        _ path: PathComponent...,
        maxFrameSize: WebSocketMaxFrameSize = .`default`,
        onUpgrade: @escaping (Request, WebSocket) -> ()
    ) -> Route {
        return self.on(.GET, path) { request -> Response in
            let res = Response(status: .switchingProtocols)
            res.upgrader = .webSocket(maxFrameSize: maxFrameSize, onUpgrade: { ws in
                onUpgrade(request, ws)
            })
            return res
        }
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Routing/RoutesBuilder.swift (Swift)

Code before:
public protocol RoutesBuilder {
    var defaultMaxBodySize: Int? { get }

    func add(_ route: Route)
}

extension RoutesBuilder {
    public var defaultMaxBodySize: Int? { 1_000_000 }
}

extension UUID: LosslessStringConvertible {
    public init?(_ description: String) {
        self.init(uuidString: description)
    }
}


Review comment (suggestion):
I'm not super happy that I have to add this (at least we can have a default value). If there are any ideas on how it can be removed, I'll take them.

Code after:
public protocol RoutesBuilder {
    func add(_ route: Route)
}

extension UUID: LosslessStringConvertible {
    public init?(_ description: String) {
        self.init(uuidString: description)
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Routing/RoutesBuilder+Method.swift (Swift)

Code before:
/// Determines how an incoming HTTP request's body is collected. 
public enum HTTPBodyStreamStrategy {
    /// The HTTP request's body will be collected into memory before the route handler is
    /// called. The max size will determine how much data can be collected. The default is
    /// 1,000,000 bytes, or 1 megabyte.
    ///
    /// See `collect(maxSize:)` to set a lower max body size.
    public static var collect: HTTPBodyStreamStrategy {
        return .collect(maxSize: 1_000_000)
    }

    /// The HTTP request's body will not be collected first before the route handler is called
    /// and will arrive in zero or more chunks.
    case stream

    /// The HTTP request's body will be collected into memory before the route handler is
    /// called.
    ///
    /// If a `maxSize` is supplied, the request body size in bytes will be limited. Requests
    /// exceeding that size will result in an error.
    case collect(maxSize: Int?)
}

extension RoutesBuilder {
    @discardableResult
    public func get<Response>(
        _ path: PathComponent...,
        use closure: @escaping (Request) throws -> Response
    ) -> Route
        where Response: ResponseEncodable
    {
        return self.on(.GET, path, use: closure)
    }


Review comment (suggestion):
This should go back to being `nil` so that it relies on the `defaultMaxBodySize`.

Code after:
/// Determines how an incoming HTTP request's body is collected. 
public enum HTTPBodyStreamStrategy {
    /// The HTTP request's body will be collected into memory up to a maximum size
    /// before the route handler is called. The application's configured default max body
    /// size will be used unless otherwise specified.
    ///
    /// See `collect(maxSize:)` to specify a custom max collection size.
    public static var collect: HTTPBodyStreamStrategy {
        return .collect(maxSize: nil)
    }

    /// The HTTP request's body will not be collected first before the route handler is called
    /// and will arrive in zero or more chunks.
    case stream

    /// The HTTP request's body will be collected into memory before the route handler is
    /// called.
    ///
    /// `maxSize` Limits the maximum amount of memory in bytes that will be used to
    /// collect a streaming body. Streaming requests exceeding that size will result in an error.
    /// Passing `nil` results in the application's default max body size being used. This
    /// parameter does not affect non-streaming requests.
    case collect(maxSize: ByteCount?)
}

extension RoutesBuilder {
    @discardableResult
    public func get<Response>(
        _ path: PathComponent...,
        use closure: @escaping (Request) throws -> Response
    ) -> Route
        where Response: ResponseEncodable
    {
        return self.on(.GET, path, use: closure)
Repository: vapor/vapor
File: Sources/Vapor/Request/Request+Body.swift (Swift)

Code before:
            }
        }
        
        public func drain(_ handler: @escaping (BodyStreamResult) -> EventLoopFuture<Void>) {
            switch self.request.bodyStorage {
            case .stream(let stream):
                stream.read { (result, promise) in
                    handler(result).cascade(to: promise)
                }
            case .collected(let buffer):
                _ = handler(.buffer(buffer))
                _ = handler(.end)
            case .none: break
            }
        }
        
        public func collect(max: Int? = 1 << 14) -> EventLoopFuture<ByteBuffer?> {
            switch self.request.bodyStorage {
            case .stream(let stream):
                return stream.consume(max: max, on: self.request.eventLoop).map { buffer in
                    self.request.bodyStorage = .collected(buffer)
                    return buffer
                }
            case .collected(let buffer):
                if let max = max, buffer.readableBytes > max {
                    return self.request.eventLoop.future(error: Abort(.payloadTooLarge))
                }
                
                return self.request.eventLoop.makeSucceededFuture(buffer)
            case .none:
                return self.request.eventLoop.makeSucceededFuture(nil)
            }
        }
        
        public var description: String {
            return ""
        }
    }
}


Review comment (suggestion):
I don't think we need this check here. The point of `max` is to avoid allocating too much memory. In this case, the memory has already been allocated we are just passing them a reference. 

Code after:
            }
        }
        
        public func drain(_ handler: @escaping (BodyStreamResult) -> EventLoopFuture<Void>) {
            switch self.request.bodyStorage {
            case .stream(let stream):
                stream.read { (result, promise) in
                    handler(result).cascade(to: promise)
                }
            case .collected(let buffer):
                _ = handler(.buffer(buffer))
                _ = handler(.end)
            case .none: break
            }
        }
        
        public func collect(max: Int? = 1 << 14) -> EventLoopFuture<ByteBuffer?> {
            switch self.request.bodyStorage {
            case .stream(let stream):
                return stream.consume(max: max, on: self.request.eventLoop).map { buffer in
                    self.request.bodyStorage = .collected(buffer)
                    return buffer
                }
            case .collected(let buffer):
                return self.request.eventLoop.makeSucceededFuture(buffer)
            case .none:
                return self.request.eventLoop.makeSucceededFuture(nil)
            }
        }
        
        public var description: String {
            return ""
        }
    }
}
Repository: vapor/vapor
File: Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift (Swift)

Code before:
import Foundation

public enum RangeUnit : Equatable {
    case bytes
    case custom(value: String)
    
    public func serialize() -> String {
        switch self {
            case .bytes:
                return "bytes"
            case .custom(let value):
                return value
        }
    }
}

public struct RangeHeader : Equatable {
    public let unit: RangeUnit
    public let ranges: [RequestedRange]
    
    public init(unit: RangeUnit, ranges: [RequestedRange]) {
        self.unit = unit
        self.ranges = ranges
    }
    
    public init?(requestHeader header: String) {
        let splitResult = header.split(separator: "=", omittingEmptySubsequences: false)
        guard splitResult.count == 2 else {
            return nil
        }
        let (unitStr, rangesStr) = (splitResult[0], splitResult[1])
        let ranges = rangesStr.split(separator: ",", omittingEmptySubsequences: false)
            .map(RequestedRange.from)
        if !ranges.filter({ $0 == nil }).isEmpty {
            return nil
        }
        let lowerCasedUnit = unitStr.trimmingCharacters(in: .whitespacesAndNewlines).lowercased()
        self.unit = lowerCasedUnit == "bytes"
            ? RangeUnit.bytes
            : RangeUnit.custom(value: lowerCasedUnit)
        self.ranges = ranges.compactMap { $0 }
    }

Review comment (suggestion):
These types should ideally be nested inside of `HTTPHeaders`. See https://github.com/vapor/vapor/blob/master/Sources/Vapor/HTTP/Headers/HTTPHeaders%2BContentDisposition.swift#L20 for a good example to copy. 

Code after:
import Foundation

extension HTTPHeaders {
    
    /// The unit in which `ContentRange`s and `Range`s are specified. This is usually `bytes`.
    /// See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Range
    public enum RangeUnit: Equatable {
        case bytes
        case custom(value: String)
        
        public func serialize() -> String {
            switch self {
            case .bytes:
                return "bytes"
            case .custom(let value):
                return value
            }
        }
    }
    
    /// Represents the HTTP `Range` request header.
    /// See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Range
    public struct Range: Equatable {
        public let unit: RangeUnit
        public let ranges: [HTTPHeaders.Range.Value]
        
        public init(unit: RangeUnit, ranges: [HTTPHeaders.Range.Value]) {
            self.unit = unit
            self.ranges = ranges
        }
        
        init?(directives: [HTTPHeaders.Directive]) {
            let rangeCandidates: [HTTPHeaders.Range.Value] = directives.enumerated().compactMap {
                if $0.0 == 0, let parameter = $0.1.parameter {
                    return HTTPHeaders.Range.Value.from(requestStr: parameter)
                }
                return HTTPHeaders.Range.Value.from(requestStr: $0.1.value)
            }
            guard !rangeCandidates.isEmpty else {
                return nil
            }
            self.ranges = rangeCandidates
Repository: vapor/vapor
File: Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift (Swift)

Code before:
import Foundation

public extension HTTPHeaders {
        
    enum RangeUnit : Equatable {
        case bytes
        case custom(value: String)
        
        public func serialize() -> String {
            switch self {
                case .bytes:
                    return "bytes"
                case .custom(let value):
                    return value
            }
        }
    }
    
    struct Range : Equatable {
        public let unit: RangeUnit
        public let ranges: [HTTPHeaders.Range.Value]
        
        public init(unit: RangeUnit, ranges: [HTTPHeaders.Range.Value]) {
            self.unit = unit
            self.ranges = ranges
        }
        
        public init?(httpHeaders headers: HTTPHeaders) {

Review comment (suggestion):
`public` keyword should go in front of each type definition / method.

Code after:
import Foundation

extension HTTPHeaders {
    
    /// The unit in which `ContentRange`s and `Range`s are specified. This is usually `bytes`.
    /// See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Range
    public enum RangeUnit: Equatable {
        case bytes
        case custom(value: String)
        
        public func serialize() -> String {
            switch self {
            case .bytes:
                return "bytes"
            case .custom(let value):
                return value
            }
        }
    }
    
    /// Represents the HTTP `Range` request header.
    /// See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Range
    public struct Range: Equatable {
        public let unit: RangeUnit
        public let ranges: [HTTPHeaders.Range.Value]
        
        public init(unit: RangeUnit, ranges: [HTTPHeaders.Range.Value]) {
            self.unit = unit
Repository: vapor/vapor
File: Sources/Vapor/Logging/LoggingSystem+Environment.swift (Swift)

Code before:
extension LoggingSystem {
    public static func bootstrap(from environment: inout Environment, factoryfactory: (Logger.Level) -> (String) -> LogHandler) throws {
        struct LogSignature: CommandSignature {
            @Option(name: "log", help: "Change log level")
            var level: Logger.Level?
            init() { }
        }

        // Determine log level from environment.
        let level = try LogSignature(from: &environment.commandInput).level
            ?? Environment.process.LOG_LEVEL
            ?? (environment == .production ? .notice: .info)

        // Disable stack traces if log level > debug.
        if level > .debug {
            StackTrace.isCaptureEnabled = false
        }

        // Bootstrap logger with a factory created by the factoryfactory.
        return LoggingSystem.bootstrap(factoryfactory(level))
    }

    public static func bootstrap(from environment: inout Environment) throws {
        try bootstrap(from: &environment, factoryfactory: { level in
            let console = Terminal()
            return { (label: String) in
                return ConsoleLogger(label: label, console: console, level: level)

Review comment (suggestion):
SwiftLog uses the parameter name `_ factory` for the factory. I think it would make sense to follow suit here. 

Code after:
extension LoggingSystem {
    public static func bootstrap(from environment: inout Environment, _ factory: (Logger.Level) -> (String) -> LogHandler) throws {
        let level = try Logger.Level.detect(from: &environment)

        // Disable stack traces if log level > trace.
        if level > .trace {
            StackTrace.isCaptureEnabled = false
        }

        // Bootstrap logger with a factory created by the factoryfactory.
        return LoggingSystem.bootstrap(factory(level))
    }

    public static func bootstrap(from environment: inout Environment) throws {
        try self.bootstrap(from: &environment) { level in
            let console = Terminal()
            return { (label: String) in
                return ConsoleLogger(label: label, console: console, level: level)
            }
        }
    }
}

extension Logger.Level: LosslessStringConvertible {
    public init?(_ description: String) { self.init(rawValue: description.lowercased()) }
    public var description: String { self.rawValue }