Conversation
louiseightsix
force-pushed
the
louis/kmessages-2
branch
from
04b392e to
16a4719
Compare
reidbuzby
left a comment
reidbuzby
left a comment
There was a problem hiding this comment.
Main feedback I think is just in the API that the library exposes. Here's my take on what I think the API should look like:
- Define options in protobuf as you already have them. I think this looks pretty good as is
- Extend the KProtoMessage class for your use case (ideally KSP handles the basic case here). Ideally also this wouldn't have to include the
Example.getDescriptor()in the constructor and this could be done with reflection. - Call
KExample.fromProto(protoMessage)when validating protos. I think exposingKMessagesand having to callKMessages.defaultInstance().fromProto(...)is a bit confusing (but maybe that is necessary, although I think it could be avoided?) - User can easily define a new validator option by extending an interface that looks something like:
// The generic on the ValidatorOption would help tell the validate function what to validate to/from somehow
class SomeOption : ValidatorOption<StringValidator>() {
override val optionId = "some_option"
// Not sure what the types would look like here, but ideally could be inferred from the
// type that gets passed into the ValidatorOption constructor
override fun validate(value) {
...
}
}I think we could keep all the proto validator types (like StringValidator, Int32Validator, etc) in the library and just hand write all those out so that every possible proto type is accounted for? That way users of the library only have to write new options for each validator if they want to add their own functionality?
| string string_default = 1 [ | ||
| (InMemoryType) = "String" | ||
| ]; |
There was a problem hiding this comment.
Could you omit the option entirely and have it infer the InMemoryType as a string?
| int32 int_default = 4 [ | ||
| (InMemoryType) = "int32" | ||
| ]; |
There was a problem hiding this comment.
Same here, could this type be inferred?
| class KExample : KProtoMessage(Example.getDescriptor()) { | ||
| val string_default: String by ProtoField() | ||
| val string_nonempty: String by ProtoField() | ||
| val string_maxlength: String by ProtoField() | ||
| val int_default: Int by ProtoField() | ||
| } |
There was a problem hiding this comment.
Are you thinking this could be generated by KSP in the future?
| } | ||
|
|
||
| class KExampleSubType1 : KExampleAdvanced() { | ||
| var string_nonempty: String by ProtoField() |
There was a problem hiding this comment.
What if you don't want to use the ProtoField() delegate? Could you add a custom validator here?
| intDefault = 456 | ||
| } | ||
|
|
||
| val kExample: KExample? = KMessages.defaultInstance().fromProto(validExample) |
There was a problem hiding this comment.
Do you like this better than having a fromProto extension function on KProtoMessage?
| private val disallowEmpty: Boolean = options.hasOption<DisallowEmptyOption>() | ||
| private val maxLength: Int? = options.getOption<MaxLengthOption>()?.maxLength | ||
|
|
||
| override fun validate(value: String): ValidationResult { | ||
| if (disallowEmpty && value.isEmpty()) | ||
| return ValidationResult.Invalid("Empty strings not allowed, but string was empty") | ||
|
|
||
| if (maxLength != null && value.length > maxLength) | ||
| return ValidationResult.Invalid( | ||
| """Max string length is [${maxLength}], | ||
| but string length was [${value.length}]""".trimMargin() | ||
| ) | ||
|
|
||
| return ValidationResult.Valid |
There was a problem hiding this comment.
Do you think there's a way to automatically check each option without having to hand write out each one? I'm thinking if someone wants to add a new String validator option like UUIDOnly or something wouldn't they also have to update this class? Which would currently not be possible since this lives in the library?
| import kotlin.reflect.KProperty | ||
|
|
||
| abstract class KProtoMessage( | ||
| private val descriptor: Descriptor, |
There was a problem hiding this comment.
Could the proto message type be passed into the constructor and the descriptor could be pulled off of the type with reflection? So the api would look like:
class KExample : KProtoMessage<Example>() {
...
}| KMessages().apply { | ||
| registerValidator(StringValidator::class) | ||
| } |
There was a problem hiding this comment.
If the registered validators is going to be a hardcoded set of validators that lives only in the library, could we just define the validators and validatorOptions as a hardcoded list? That way you don't have to call KMessages.defaultInstance() ever?
2 participants
No description provided.