[Discussion] OpenAPIKit

[tanner0101 (Tanner)]

Thanks for the detailed proposal! ++++1 to this overall.

I've only been able to review the code so far. I'm hoping to get a chance soon to dig into this further and try building some stuff with it. As is though, I think the idea and execution of this package is strong and should move forward.

One general tip I'd recommend for any package going through the proposal process (no specific cause for concern here) is to do a public API audit. Basically search everywhere the public keyword exists in code and consider whether that is essential API. Keeping the API surface as small as possible makes it easier to change and improve the package once it's released.

Below are some comments based on code review:

---

The JSONReference stuff is a bit hard for me to understand from glancing at the code. I'm not quite sure what these things do:

RefDict<Root: ReferenceRoot, Name: RefName, RefType: Equatable & Codable>: ReferenceDict, Equatable 

public protocol RefName {
    static var refName: String { get }
}

public protocol ReferenceRoot: RefName {}

public protocol AbstractReferenceDict: RefName {
    func contains(_ key: String) -> Bool
}

public protocol ReferenceDict: AbstractReferenceDict {
    associatedtype Value
}

Either<JSONReference<OpenAPI.Components, OpenAPI.Request>, OpenAPI.Request>

Some explanation about the reasoning behind these types and what they are doing in the proposal would be great. Any way to make the code more obvious would be awesome as well, though I understand OpenAPI is quite complex so that may not be possible.

---

@_exported should be avoided (Vapor 4 is bad at this and Vapor 5 will attempt to improve it).

@_exported import AnyCodable

This code prevents OpenAPIKit from moving to a new major version of AnyCodable without breaking. (This will be irrelevant if you end up removing the AnyCodable dependency of course).

---

I've noticed lots of abbreviations in type names, e.g., AbstractReferenceDict. Swift style recommends not using abbreviations.

Avoid abbreviations. Abbreviations, especially non-standard ones, are effectively terms-of-art, because understanding depends on correctly translating them into their non-abbreviated forms.

---

Some responses to specific parts of the proposal:

@mattpolzin

I would be willing to re-implement (mostly copy) support for Either from Poly into OpenAPIKit if removing Poly as a dependency is seen as more beneficial than reducing the footprint of the OpenAPIKit codebase.

@mattpolzin

This is yet another dependency I would be willing to pull into OpenAPIKit -- I know it is not uncommon to roll-your-own support for this anyway.

I'm +1 to pulling the min required elements of these into OpenAPIKit itself. Given that this is a very low level package that will be brought into potentially many libraries, keeping this as simple as possible will be a huge win.

---

@mattpolzin

The OrderedDictionary library just offers up one type and I bet you can guess what that is. Ordering of hashes is important to OpenAPIKit for two reasons:

Could any usages of OrderedDictionary be replaced with arrays or arrays of tuples? Performance isn't critical for this package so I don't think array lookups (O(n) vs. dictionary O(1)) would be a huge issue. We could always add caching if we needed to improve performance. I'd be interested for the opinions of others here.