Remove unused APIs for removal to clarify responsibility/intent #4156
Conversation
niloc132
added
NoDocumentationNeeded
groovy
ReleaseNotesNeeded
|
|
||
| /** | ||
| * Sets the scriptPathLoader that is in use for this session. | ||
| * | ||
| * @param scriptPathLoader a supplier of a script path loader | ||
| * @param caching whether the source operation should cache results | ||
| */ | ||
| void setScriptPathLoader(Supplier<ScriptPathLoader> scriptPathLoader, boolean caching); | ||
| @Deprecated(forRemoval = true) | ||
| default void setScriptPathLoader(Supplier<ScriptPathLoader> scriptPathLoader, boolean caching) { |
There was a problem hiding this comment.
This seems like something enterprise might need.
There was a problem hiding this comment.
We probably do need to integrate with this, because right now the workers are not getting proper source.
| @@ -16,6 +16,7 @@ | |||
| * This class represents uses of the source() and sourceOnce() method calls. It will use the underlying scriptPathLoader | |||
| * to get the script text to use. | |||
| */ | |||
| @Deprecated(forRemoval = true) | |||
There was a problem hiding this comment.
This is an enterprise thing, but I think it might be best to delete it. We should discuss with @cpwright .
There was a problem hiding this comment.
You are using it too from what I can tell.
setVariable("source", sourceClosure = new SourceClosure(this, pathLoader, false, caching));
setVariable("sourceOnce", sourceOnceClosure = new SourceClosure(this, pathLoader, true, false));
I think that Groovy source is kind of terrible in a lot of ways. It is our own invention and has some unpleasant semantics when you mix the code between the sourced thing and the rest of the code. I don't know if we can do something better given the natural language constructs, because I am not a Groovy expert.
I think Raffi and Pete might want to kick us in the shins if we take it away without a suitable replacement though.
There was a problem hiding this comment.
You are using it too from what I can tell.
That would be true if setScriptPathLoader was ever called - there is one callsite, from GroovyDeephavenSession.onApplicationInitializationBegin, which itself is never called, so we stick with the default impl in groovy:
source = {fileName ->
__groovySession.runScript(fileName)
}
sourceOnce = {fileName ->
__groovySession.runScriptOnce(fileName)
}
After some discussion with Charles and Raffi, I'm going to create a DHC and DHE issue to track this further, but the simple consensus is that we're going to deprecate/remove source and sourceOnce, and instead encourage using Groovy's own language features for this sort of thing, like import, and potentially a custom classloader to find resources not on the usual classpath (i.e. a script/notebook directory, etc). Re-adding source/sourceOnce (or something like it) could be done in a given installation at the discretion of local admins, but shouldn't be strictly necessary to use Groovy as it is "intended" to be used.
|
What is your policy about deprecation marks vs. just doing it? |
|
Deprecation: Mostly I'm hoping to get in a better habit project-wide of not just deleting things and breaking APIs. In this case I'm nearly certain we could get away with deleting everything (except maybe source/sourceOnce), and have a second commit ready to do that if we would prefer it. |
|
I'm sure users don't use this stuff. We should just delete it, if it's not going to break DnD. I'm happy to get rid of |
niloc132
changed the title
I have confirmed that none of these are used in DnD, and have a follow-up commit ready to delete them entirely. They are unused in DHC itself, and unimplemented in Python anyway.