Kotlin / kotlin-jupyter

Kotlin kernel for Jupyter/IPython
Apache License 2.0
1.12k stars 107 forks source link

classpath resource loading uses wrong validation check #389

Closed jbaron closed 1 year ago

jbaron commented 1 year ago

When specifying a classpath resource in my JupyterIntegration implementation, I always get a Warning (even although it is loaded fine and I provided the right location). The reason is that the current check (see below) is likely not correct.

private fun checkClassPath(classPath: String) {
        if (javaClass.getResource(classPath) == null) {
            Logger.getLogger("JupyterIntegration").warning("A resource with classpath '$classPath' not found.")
        }
}

Problem is that the resource is in another module from the code performing the check and so javaClass.getResource(classPath) will not find it. I would like to propose to remove this check altogether, since otherwise it is just require duplicating the logic of the actually resource loader itself.

If required I can create a PR for this (I noticed there are no unit tests for this part, so should be easy :)

see also discussion on: Slack Discussion

ileasile commented 1 year ago

Hello! Yes, sure, I would like to review your PR on this. I agree that this check should be deleted. But possibly it's better to add this check to inheritors of org.jetbrains.kotlinx.jupyter.libraries.LibraryResourcesProcessor. Now they silently fail if there is no corresponding resource.

jbaron commented 1 year ago

Did a quick check and should not be hard to implement.

One question: current logic only tries alternative defined locations if there is an IOException. Is it desirable if the CLASSPATH_PATH loading fails, still to try alternatives if any (like LOCAL_PATH) (so throw perhaps an IOException)?

val resource = classLoader.getResource(path)
resource != null && return CodeScriptModifierFunctionGenerator(resource.readText())
throw IOException("resource $path not found on classpath")

Also just created PR that uses above approach.

ileasile commented 1 year ago

Yes, I think that we should try all the alternatives we have. Thank you for PR, merged!