Make parameters classes easier to work with #509
Conversation
The key change is changing the type of `BackfillRegistration.parametersClass` from `KClass<Any>` to `KClass<out Any>`. This is a clearer expression of the intent to allow backfill parameters to be of any type. In the previous implementation, the type was `KClass<Any>` which actually means the class of the `Any` type. This required some gymnastics for the standard clients to be able to register themselves because they had to cast their parameters type to `Any` (usually by casting the backfill class to something like `SomeBackfill<Any, Any>` so that a parameters type of `Any` gets extracted). With this change, a lot of these type gymnastics can be simplified. This also makes it much more straightforward to implement a custom client that uses a custom parameters type.
| @@ -60,14 +63,14 @@ class JooqBackend @Inject constructor( | |||
| } | |||
| } | |||
|
|
|||
| private fun <T : Any> parametersClass(backfillClass: KClass<out JooqBackfill<*, T>>): KClass<T> { | |||
There was a problem hiding this comment.
Does removing the generic mean the type isn't known at the callsite?
There was a problem hiding this comment.
Before, we were always casting the type to Any so that it was "known" at the callsite. This was all to satisfy the type of KClass<Any> on BackfillRegistration.parametersClass. Using the type KClass<Any> meant you couldn't use any params type without casting it to Any first
| BackfilaParametersOperator(parametersClass(backfill::class)), | ||
| BackfilaParametersOperator( | ||
| parametersClass(backfill::class), | ||
| ) as BackfilaParametersOperator<Param>, |
There was a problem hiding this comment.
Why do we end up needing to cast here and didn't before?
There was a problem hiding this comment.
We were casting to S3DatasourceBackfill<Any, Any> in create before (just below here). This allowed the Params type to always come through as KClass<Any> to satisfy the previous type of BackfillRegistration.parametersClass
There was a problem hiding this comment.
The constructor is actually important too:
https://github.com/cashapp/backfila/blob/master/client-base/src/main/kotlin/app/cash/backfila/client/spi/BackfilaParameters.kt#L67
Anyway it is an interesting point. I'm not entirely happy with the gymnastics and open to doing it another way as long as its type safe and keeps the complexity in the clients and out of the backfills themselves.
I'll think about it some more
| datastore = datastore, | ||
| parametersOperator = BackfilaParametersOperator(parametersClass(backfill::class)), | ||
| ) | ||
| @Suppress("UNCHECKED_CAST") |
There was a problem hiding this comment.
Do we need this? I don't know that I like casting the operator, I'd rather cast the Param type exclusively.
There was a problem hiding this comment.
Before we were always creating this with a parameters class of Any. Here we are casting to the actual parameters type
|
|
||
| private fun <T : Any> parametersClass(backfillClass: KClass<out FixedSetBackfill<T>>): KClass<T> { | ||
| private fun parametersClass(backfillClass: KClass<out FixedSetBackfill<*>>): KClass<*> { |
There was a problem hiding this comment.
Yeah I don't think that removing this anchoring to the type is what I'm looking for.
I don't want parameters to be able to be any type for a particular backfill but only the parameterized type for that specific backfill.
There was a problem hiding this comment.
You still get that here. Try it for yourself, the types still have to match up
This change shouldn't affect how that code works |
The key change is changing the type of
BackfillRegistration.parametersClassfromKClass<Any>toKClass<out Any>. This is a clearer expression of the intent to allow backfill parameters to be of any type.In the previous implementation, the type was
KClass<Any>which actually means the class of theAnytype. This required some gymnastics for the standard clients to be able to register themselves because they had to cast their parameters type toAny(usually by casting the backfill class to something likeSomeBackfill<Any, Any>so that a parameters type ofAnygets extracted).With this change, a lot of these type gymnastics can be simplified. This also makes it much more straightforward to implement a custom client that uses a custom parameters type.