This looks great! Huge +1 to this proposal and the desired sandbox maturity level.
I have some comments on the proposal below. It's mostly just small things that I think could make the API more consistent w/ Swift and NIO. But, as it stands, I think this proposal is good to move forward regardless of whether these changes are made.
Thanks, @Mordil !
RESPTranslator as a struct
We may want to add some configuration options for RESP translation in the future. If RESPTranslator were a struct with instance methods, we could pass those options in during init without needing to change the parsing / serialization API. This would give us more flexibility in the future.
RESPTranslator.parseBytes using position
Changing the current reader index of a ByteBuffer is a very cheap operation. Generally, you should always use the read... prefixed methods that mutate a buffer instead of the get methods plus a custom offset.
Because of this, I think we should expect that the buffer supplied to RESPTranslator.parseBytes has the message as the first readable byte, so that we can omit the fromIndex: inout Int. This would simplify the API by relying on the great work the NIO team has already done in ByteBuffer.
RESPValueConvertible API
I think this package could benefit from a convertible-protocol pattern that Vapor's DB modules are using:
public protocol FooConvertible {
init?(foo: Foo)
var foo: Foo? { get }
}
There are a few key differences with this pattern compared to RESPValueConvertible:
1: The init method uses a label. Given that very heavily used types like String and Int will conform, we should probably avoid overloading method names like init(_:) too much.
2: The convert-to method allows returning nil if the conversion fails (just like the init method does)
3. This more closely matches patterns already found in Swift like CustomStringConvertible.
Bootstrap helpers as extensions on NIO types
NIO at least offers bootstrap helpers (like addHTTPHandlers()) as methods on NIO types like ChannelPipeline. Perhaps this package should consider matching that instead of using static methods on the Redis type.
sub-class vs. protocol
I'd personally rather see RedisCommandHandler being a protocol w/ default extensions than an open class. IMO inheritance is only really appropriate for Objective-C compatible development (like w/ UIKit, AppKit, etc). With pure Swift, protocols + extensions can do the same job, while also being composable.
Redis.makeConnection
I think this method would make more sense on RedisConnection itself. If I were attempting to create a new RedisConnection in my code (without reading the docs first), I would probably write something like:
import RedisNIO
let conn = RedisConnection.
After writing this, I'd inspect the auto-completion options available. At the current state, I'd see the init(channel:logger:) method which I might think to use, but not the actual method I need.
If the makeConnection (maybe call it connect?) method were statically available on RedisConnection, I would find it immediately.
Potentially init(channel:logger:) could consider being made internal, too. There's really no reason to use it outside of testing, where one could use @testable import anyway.
) - I'll update to include a label
).
should we lock this thread? or further discussion needed?