Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

koperagen
Copy link
Collaborator

It's not new to have KType / KClass overloads next to inline reified APIs, so i believe this addition would be an easy one.

@koperagen koperagen added the enhancement New feature or request label Aug 19, 2024
@koperagen koperagen added this to the 0.14.0 milestone Aug 19, 2024
@koperagen koperagen self-assigned this Aug 19, 2024
Copy link
Contributor

Generated sources will be updated after merging this PR.
Please inspect the changes in here.

Copy link
Collaborator

@Jolanrensen Jolanrensen left a 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<*> =
Copy link
Collaborator

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?

Copy link
Collaborator

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>.

Copy link
Collaborator

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?

Copy link
Collaborator Author

@koperagen koperagen Aug 20, 2024

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

Copy link
Collaborator

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.

@Jolanrensen Jolanrensen modified the milestones: 0.14.0, 0.15.0 Sep 23, 2024
@koperagen
Copy link
Collaborator Author

val entityManager = springContext.getBean(EntityManager::class.java)
entityManager.createQuery("select e from Owner e").resultList.toDataFrame()

another case when Iterable has Any? type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants