-
Notifications
You must be signed in to change notification settings - Fork 92
Implement ScriptModule functions for Java HashMap #336
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
Conversation
With commit b1965c6 I fixed the outputs of `run_script` and `run_macro` by returning the promised Java HashMap instead of an instance of a `ScriptModule`. This change however (1) changes the API behavior and (2) breaks any script that depends on the `getOutput()` and `getOutputs` calls. To alleviate this headache I made, we agreed to implement these methods on the Java HashMap, thereby not breaking scripts calling for outputs. However the `getInput` and `getInputs` functions are not implemented as they would exisit in a different Hash map. Calling these will raise a `NotImplementedError`. This commit also adds the appropriate tests.
|
Maybe calling |
|
Great idea, I'll that in! |
|
The |
The SciJava ScriptModule interface hierarchy has quite a few methods. Of course, many of them do not make sense to call anymore after actually running the module. But for the sake of compatibility, this commit changes run_script to return a dict-extending object that also supports all the public non-deprecated ScriptModule methods, delegating to the linked ScriptModule instance while also issuing a deprecation warning.
ctrueden
left a comment
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.
Thanks for following through on this, @elevans!
I took the liberty of pushing this a bit farther: with the commit I just pushed, the run_script function now returns a dict-extending object that offers the entire public ScriptModule API, delegating to the linked ScriptModule object's corresponding method, but also printing a deprecation warning in so doing. On my side, this is now good to go, as it avoids backwards-incompatible API breakage. Could you take a look, and merge if you like it? Thanks.
|
Awesome thank you! I'm looking now and will merge after (unless something jumps out). |
With commit b1965c6 I fixed the outputs of
run_scriptandrun_macroby returning the promised Java HashMap instead of an instance of aScriptModule. This change however (1) changes the API behavior and (2) breaks any script that depends on thegetOutput()andgetOutputs()calls. To alleviate this headache I made, we agreed to implement these methods on the Java HashMap, thereby not breaking scripts calling for outputs. However thegetInput()andgetInputs()functions are not implemented as they would exist in a different Hash map. Calling these will raise aNotImplementedError.I also added the appropriate tests for the
JavaHashMapAddons.Here is a minimal script to test/play with this behavior:
One question is if raising a
NotImplementedErroris appropriate here or if we should do something more fancy.