DGM.println(Object) and friends do a Writer.close() if the object is a writer. Should be flush() instead.

Fix committed - http://fisheye.codehaus.org/changelog/groovy/?cs=14638 + http://fisheye.codehaus.org/changelog/groovy/?cs=14639

Will merge to 1.5 and 1.6 if no objection.

To call flush() is the wrong thing either. It is a side effect the user does not want to have. Think e.g. of socket streams where frequent flushes may decrease network throughput. java.io.PrintWriter has auto-flush turned off by default.

It seems unneccessary to construct a PrintWriter within the DGM methods. The print(ln) methods could use Writer#write(InvokerHelper.toString(value)) followed by an optional new-line Writer#write(System.getProperty("line.separator")).

I don't have a problem the flush's effect on the underlying writer - this is a convenience method and the user could easily change their code to improve performance.

I do agree that just using write is better, but I didn't have more time to spend on this issue last night which is why I just fixed the existing approach (creating a PrintWriter). I want to check the implementation of PrintWriter itself and find the best way to Groovy to cache the line.separator property (which it may do already).

I also have an enhancement (which will be a separate issue) for the non-Writer case so that it will also look for the Script 'out' property (which will also then work with the JSR-223 script engine writer).

Note that PrintWriter#flush() does nothing but flushing the underlying stream. When it is constructed with a Writer PrintWriter it has no internal buffer. So calling flush() does not improve anything.

Additionally, when calling Writer methods like Writer#write(...) the user is getting the benefit of receiving an exception if something goes wrong. The wrapping PrintWriter would swallow them what is really bad.

Ah, good point about PrintWriter.flush being a no-op wrt the PW when constructed over a Writer.

But as for no exception on problems with the underlying stream, that is a reason to stick with PrintWriter. The API for print/println is that they do not throw checked exceptions. Of course PrintWriter then has checkError and Writer does not.

I'll remove the flush.

Is it really a problem to throw checked exceptions from DGM#print(ln)? Other DGM methods (e.g. leftShift) do this as well. Groovy doesn't care if there's a throws keyword.

Using PrintWriter adds a lot of unneccessary overhead (object construction, synchonization, stack frames) and the caller has no chance to get a notification if something went wrong. PrintWriter's (stateful) checkError() mechanism is not available for arbitrary Writers.

I think the behavior should conform to PrintWriter.print(ln) as much as possible. This fix was prompted by these methods closing the underlying Writer, which is clearly bad (and a sign it probably is very little used - which brings up another interesting idea - automated usage profiling and feedback).

As for performance, this is a convenience method and I'm more interested in brevity and clarity than a constructor call or two. Anyone who is calling this in a loop can use 'new PrintWriter(w).withWriter' or such as they prefer.

OK.

BTW the following test case should probably run through (it does not in 1.5.6) http://jira.codehaus.org/browse/GROOVY-3237.

is this issue really a must for 1.6?

Certainly. Those methods are seriously broken if printing to something like a file which only gets the first print. Folks maybe haven't noticed this because of the habit of printing to a StringWriter which ignores close() calls.

so what about the issue? The fix is done... what is missing?

Committed to 1.6 (cs14853).

Committed to 1.5 (14854).

The local build on my windows XP machine is failing because of Groovy3235Bug.testBug(). You may want to check. Below is the error details that show up in the log:

Caused an ERROR
Expression: (d == t). Values: d = This is one line.
This is another.
All these lines should be written.
, t = This is one line.
This is another.
All these lines should be written.

java.lang.AssertionError: Expression: (d == t). Values: d = This is one line.
This is another.
All these lines should be written.
, t = This is one line.
This is another.
All these lines should be written.

at org.codehaus.groovy.runtime.InvokerHelper.assertFailed(InvokerHelper.java:373)
at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.assertFailed(ScriptBytecodeAdapter.java:658)
at groovy.bugs.Groovy3235Bug.testBug(Groovy3235Bug.groovy:18)

I have verified both on 1.5.x and 1.6.x dev branches and both builds are failing for me.