-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add toDataFrame(KClass) overload to convert type erased lists #825
base: master
Are you sure you want to change the base?
Conversation
Generated sources will be updated after merging this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea but missing some typing and overloads I think
@@ -28,6 +28,11 @@ public inline fun <reified T> Iterable<T>.toDataFrame(): DataFrame<T> = | |||
properties() | |||
} | |||
|
|||
public fun Iterable<*>.toDataFrame(klass: KClass<*>): DataFrame<*> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add some small KDocs for new public functions :) Also, maybe people want to specify roots/maxdepth as well, would that be possible here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, since we supply a KClass<T>
we can get the type information. If we have an Iterable<Any?>
, we know we'll get a DataFrame<T>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally, would it make sense to offer overloads with body: CreateDataFrameDsl<T>.() -> Unit
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, since we supply a KClass we can get the type information. If we have an Iterable<Any?>, we know we'll get a DataFrame.
If you have KClass with specific T, i'd say you might as well use list.toDataFrame() without KClass?
Finally, would it make sense to offer overloads with body: CreateDataFrameDsl.() -> Unit too?
Well, this overload is CreateDataFrameDsl<T>.() -> Unit = { properties() }
so we'd need to combine them. Also it's supposed to be used when only runtime KClass<*> is available. What useful can be done with CreateDataFrameDsl<Any?>.() -> Unit
?
Also, maybe people want to specify roots/maxdepth as well, would that be possible here?
roots - no, because Any?. maxDepth - yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have KClass with specific T, i'd say you might as well use list.toDataFrame() without KClass?
Hmm, I guess you're right. I first thought it might be possible to use another class as argument of toDataFrame()
, then it would make sense to get T
... However, that results in java.lang.IllegalArgumentException: object is not an instance of declaring class
. It might be worth mentioning in attached KDocs that all elements in the iterable must be exact instances the KClass given.
another case when Iterable has Any? type |
It's not new to have KType / KClass overloads next to inline reified APIs, so i believe this addition would be an easy one.