Skip to content

Conversation

@Squareys
Copy link
Contributor

@Squareys Squareys commented Oct 8, 2015

Hi @ctrueden !

here is the pullrequest promised in scijava/scripting-jython#6. My solution changes ScriptInfo as follows:

Instead of ScriptInfo.getReader() returning the reader passed onto the constructor of ScriptInfo, the constructor now reads the entire contents of the Reader into a string, so that ScriptInfo.getReader() can return a new StringReader.

Greetings,
Squareys

ScriptModule, for example, does not reset the Reader aquired via
`ScriptInfo.getReader()`. While it is definitely an option to fix this
issue by resetting it there, this way we can ensure that the behaviour of
`ScriptInfo.getReader()` stays consistent. Also, this solution makes way
to some day maybe run ScriptModules in parallel.

Signed-off-by: Squareys <Squareys@googlemail.com>
Signed-off-by: Squareys <Squareys@googlemail.com>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This number is pretty arbitrary. Any ideas on that? Use the probably also arbitrary (but alot bigger) PARAM_CHAR_MAX instead? :)

This code is based off of the code I wrote in scripting-java a while ago.

@Squareys
Copy link
Contributor Author

It would be great, if someone could quickly review this, since this would fix scijava/scripting-jython#6.

ctrueden added a commit that referenced this pull request Oct 12, 2015
@ctrueden ctrueden merged commit f1d2b4d into scijava:master Oct 12, 2015
@ctrueden
Copy link
Member

Thanks @Squareys. In retrospect, the design of the ScriptInfo is pretty bad. I'm sorry about that. Your changes are a nice patch to the issue. It is better to just work with String always, and forget the whole Reader thing, since script size does not need to scale beyond very small values.

The 8192 value is fine for now. Perhaps some day SJC will depend on Guava and then we can call a utility method (CharStreams.toString) to read the data in a more succinct way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants