Re: [groovy-scm] [18132] trunk/groovy/groovy-core/src/main/groovy/lang: (GROOVY-3856) Add Reader methods

[Jochen Theodorou]

I think this change needs a bit discussion...


user57@... schrieb:
[...]
> (GROOVY-3856 <http://jira.codehaus.org/secure/ViewIssue.jspa?key=GROOVY-3856>) Add Reader methods

and issue is given, but the motivation remains unclear and there is no
test, which will cause questions like "is that used"...

[...]
> + public GroovyCodeSource(InputStream inputStream, String name, String codeBase) throws UnsupportedEncodingException {
> +        this(new InputStreamReader(inputStream, "UTF-8"), name, codeBase);
> +    }

Before the text was read into a string using
DefaultGroovyMethods.getText(inputStream). This uses the default
encoding stored in file.encoding, while the change here uses UTF-8
always. This could be wrong. So I think thee encoding here needs to be
removed. It is a deprecated method exactly because of the encoding
issue, still I think changing that without need is no good.

[...]

> +    public Reader getReader() {
> +        if (file == null) {
> +            return new StringReader(scriptText);
> +        }
> +        else {
> +            try {
> +                return new BufferedReader(new FileReader(file));
> +            }
> +            catch (FileNotFoundException e) {
> +                throw new RuntimeException("Impossible to read from the associated script: " + file + " with name: " + name);
> +            }
> +        }
> +    }

well and here... what is the use case for this?

[...]

> @@ -450,7 +452,7 @@
>      public Object run(final String scriptText, final String fileName, String[] args) throws CompilationFailedException {
>          GroovyCodeSource gcs = (GroovyCodeSource) AccessController.doPrivileged(new PrivilegedAction() {
>              public Object run() {
> -                return new GroovyCodeSource(scriptText, fileName, "/groovy/shell");
> +                return new GroovyCodeSource(scriptText, fileName, DEFAULT_CODE_BASE);
>              }
>          });
>          Class scriptClass = parseClass(gcs);
> @@ -463,7 +465,20 @@
>       * @param in       the stream reading the script
>       * @param fileName is the logical file name of the script (which is used to create the class name of the script)
>       * @param args     the command line arguments to pass in
> +     */
> +    public Object run(final Reader in, final String fileName, String[] args) throws CompilationFailedException {
> +        GroovyCodeSource gcs = new GroovyCodeSource(in, fileName, DEFAULT_CODE_BASE);
> +        Class scriptClass = parseClass(gcs);
> +        return runScriptOrMainOrTestOrRunnable(scriptClass, args);
> +    }

comparing those two methods I have first to say: "yet another run
method?" The same applies ot the evaluate and parse variants.

And next is the question why the string version needs a proveledged
block and the reader version not. Either both or none, and when I think
of the seurity check in the constrcutor in GroovyCodeSource I tend to
say that the block is needed

bye blackdrag

--
Jochen "blackdrag" Theodorou
The Groovy Project Tech Lead (http://groovy.codehaus.org)
http://blackdragsview.blogspot.com/


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Jason Dillon]

On Oct 28, 2009, at 9:34 PM, Jochen Theodorou wrote:
>> (GROOVY-3856 <http://jira.codehaus.org/secure/ViewIssue.jspa?key=GROOVY-3856 
>> >) Add Reader methods
>
> and issue is given, but the motivation remains unclear and there is  
> no test, which will cause questions like "is that used"...

I can add some tests.

> [...]
>> + public GroovyCodeSource(InputStream inputStream, String name,  
>> String codeBase) throws UnsupportedEncodingException {
>> +        this(new InputStreamReader(inputStream, "UTF-8"), name,  
>> codeBase);
>> +    }
>
> Before the text was read into a string using  
> DefaultGroovyMethods.getText(inputStream). This uses the default  
> encoding stored in file.encoding, while the change here uses UTF-8  
> always. This could be wrong. So I think thee encoding here needs to  
> be removed. It is a deprecated method exactly because of the  
> encoding issue, still I think changing that without need is no good.

Aighty, will put it back.

> [...]
>> +    public Reader getReader() {
>> +        if (file == null) {
>> +            return new StringReader(scriptText);
>> +        }
>> +        else {
>> +            try {
>> +                return new BufferedReader(new FileReader(file));
>> +            }
>> +            catch (FileNotFoundException e) {
>> +                throw new RuntimeException("Impossible to read  
>> from the associated script: " + file + " with name: " + name);
>> +            }
>> +        }
>> +    }
>
> well and here... what is the use case for this?

Dunno, just added Reader methods for everything that had InputStream.  
Though looks like GCS.getInputStream() isn't used.

> [...]
>> @@ -450,7 +452,7 @@
>>     public Object run(final String scriptText, final String  
>> fileName, String[] args) throws CompilationFailedException {
>>         GroovyCodeSource gcs = (GroovyCodeSource)  
>> AccessController.doPrivileged(new PrivilegedAction() {
>>             public Object run() {
>> -                return new GroovyCodeSource(scriptText, fileName,  
>> "/groovy/shell");
>> +                return new GroovyCodeSource(scriptText, fileName,  
>> DEFAULT_CODE_BASE);
>>             }
>>         });
>>         Class scriptClass = parseClass(gcs);
>> @@ -463,7 +465,20 @@
>>      * @param in       the stream reading the script
>>      * @param fileName is the logical file name of the script  
>> (which is used to create the class name of the script)
>>      * @param args     the command line arguments to pass in
>> +     */
>> +    public Object run(final Reader in, final String fileName,  
>> String[] args) throws CompilationFailedException {
>> +        GroovyCodeSource gcs = new GroovyCodeSource(in, fileName,  
>> DEFAULT_CODE_BASE);
>> +        Class scriptClass = parseClass(gcs);
>> +        return runScriptOrMainOrTestOrRunnable(scriptClass, args);
>> +    }
>
> comparing those two methods I have first to say: "yet another run  
> method?" The same applies ot the evaluate and parse variants.

Right so I have to use the InputStream variants for different  
purposes, don't use the String or File versions at all.  ANd now that  
the IS versions are deprecated, saying use the Reader versions, I  
figured it would make sense to actually have Reader versions.


> And next is the question why the string version needs a proveledged  
> block and the reader version not. Either both or none, and when I  
> think of the seurity check in the constrcutor in GroovyCodeSource I  
> tend to say that the block is needed

I agree will put it in there.

--jason


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Jochen Theodorou]

Jason Dillon schrieb:
[...]

>> [...]
>>> +    public Reader getReader() {
>>> +        if (file == null) {
>>> +            return new StringReader(scriptText);
>>> +        }
>>> +        else {
>>> +            try {
>>> +                return new BufferedReader(new FileReader(file));
>>> +            }
>>> +            catch (FileNotFoundException e) {
>>> +                throw new RuntimeException("Impossible to read from
>>> the associated script: " + file + " with name: " + name);
>>> +            }
>>> +        }
>>> +    }
>>
>> well and here... what is the use case for this?
>
> Dunno, just added Reader methods for everything that had InputStream.  
> Though looks like GCS.getInputStream() isn't used.

it is not used because it is a deprecated method anyway. All the
InputStream taking methods are deprecated. Instead the String taking
methods are to be prefered. I am not sure we then need the Reader logic.
I hate it if APIs explode just because variants are added with the
thought of alinging something, and then that stuff is not even used. In
fact I am for removing the reader stuff unless there is a use case for
it that cannot be done using the String approach (which I doubt) or
which makes the handling using String very complicated.

The only advantage of the Reaer approach could be that you don't need to
keep the source in memory, but if the implementation just reads the text
in and then does a compilation with the string in the end, then I don't
see any benefit.


[...]
> ANd now that the IS
> versions are deprecated, saying use the Reader versions, I figured it
> would make sense to actually have Reader versions.

why don't you use the String version? As it currently is I would vote
for undoing that commit.

bye blackdrag

--
Jochen "blackdrag" Theodorou
The Groovy Project Tech Lead (http://groovy.codehaus.org)
http://blackdragsview.blogspot.com/


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Jason Dillon]

On Oct 29, 2009, at 9:49 PM, Jochen Theodorou wrote:
>>>
>> Dunno, just added Reader methods for everything that had  
>> InputStream.  Though looks like GCS.getInputStream() isn't used.
>
> it is not used because it is a deprecated method anyway.

How can you say it is unused?  Its like GroovyShell.initializeBinding
() perhaps unused internally, but its public so who knows.  It  
certainly appears useless  though.


> All the InputStream taking methods are deprecated. Instead the  
> String taking methods are to be prefered. I am not sure we then need  
> the Reader logic. I hate it if APIs explode just because variants  
> are added with the thought of alinging something, and then that  
> stuff is not even used.

Give me a break about exploding apis, Groovy is an API leaking  
machine.  Its got a ton of optional goodies (someone might need/want/
use) but I will probably never need them.

And I would not even really care if I didn't actually want those  
Reader methods!  Guillaume mentioned he was looking for them as well.

> In fact I am for removing the reader stuff unless there is a use  
> case for it that cannot be done using the String approach (which I  
> doubt) or which makes the handling using String very complicated.

I think String and Reader is to be expected input for integration  
layers for scripting interfaces, ie. javax.script.ScriptEngine exposes  
both... BTW does not expose file, which IMO is a function of a  
FileReader, optionally buffered as the caller sees fit.  Exposing a  
File version is akin to exposing an InputStream version that simply  
wraps into an InputStreamReader and uses the Reader methods... purely  
for ease of use.


> The only advantage of the Reaer approach could be that you don't  
> need to keep the source in memory, but if the implementation just  
> reads the text in and then does a compilation with the string in the  
> end, then I don't see any benefit.

Right well, as I said before I think that is fundamentally broken.

--jason

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email