groovy

leftShift and append on File only work with Strings

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 1.0-JSR-5
  • Fix Version/s: 1.0-JSR-6
  • Component/s: groovy-jdk
  • Labels:
    None
  • Number of attachments :
    3

Description

This means that writing a Writable object to a file using << or append() does not work on Writable objects

Attachments

  1. Text File
    DefaultGroovyMethods.patch
    23/Jun/06 10:37 AM
    2 kB
    Joachim Baumann
  2. Text File
    DefaultGroovyMethods.patch
    23/Jun/06 7:31 AM
    2 kB
    Joachim Baumann
  3. File
    FileAppendTest.groovy
    26/Jun/06 2:49 PM
    4 kB
    Joachim Baumann

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Joachim Baumann added a comment -

Here is a patch that implements the functionality for objects, and differentiates between Writables (calling the writeTo()-method) and others (calling the toString()-method if necessary) ...

Only DefaultGroovyMethods.java is involved.

Cheers, Joe

5273c5273
< public static File leftShift(File file, String text) throws IOException {
—
> public static File leftShift(File file, Object text) throws IOException {
5299,5302c5299,5305
< public static void append(File file, String text) throws IOException {
< BufferedWriter writer = newWriter(file, true);
< writer.write(text);
< writer.close();
—
> public static void append(File file, Object text) throws IOException {
> BufferedWriter writer = newWriter(file, true);
> if(text instanceof Writable)
> ((Writable)text).writeTo(writer);
> else
> writer.write(text.toString());
> writer.close();
5304d5306
<

Show
Joachim Baumann added a comment - Here is a patch that implements the functionality for objects, and differentiates between Writables (calling the writeTo()-method) and others (calling the toString()-method if necessary) ... Only DefaultGroovyMethods.java is involved. Cheers, Joe 5273c5273 < public static File leftShift(File file, String text) throws IOException { — > public static File leftShift(File file, Object text) throws IOException { 5299,5302c5299,5305 < public static void append(File file, String text) throws IOException { < BufferedWriter writer = newWriter(file, true); < writer.write(text); < writer.close(); — > public static void append(File file, Object text) throws IOException { > BufferedWriter writer = newWriter(file, true); > if(text instanceof Writable) > ((Writable)text).writeTo(writer); > else > writer.write(text.toString()); > writer.close(); 5304d5306 <
Hide
Permalink
blackdrag blackdrag added a comment -

let me see if that is right... you want to replace leftShift(File,Object) with leftShift(File,String), and you want to add a append method working on Object? Maybe I have read the diff wrong, sorry.

Anyway, some hints for writing method in DGM. First, you don't really have to use instanceof there. make two append methods, one for Writable and one for Object. Then, if you want to keep append(File,Object), you should keep the leftshift too. Maybe even adding a leftShift(File,Writable) would be good.

ah, one more.. shouldn't we close the File in case of an Exception?

Hmm, we should really remove all instanceof instructions in DGM where possible.

Show
blackdrag blackdrag added a comment - let me see if that is right... you want to replace leftShift(File,Object) with leftShift(File,String), and you want to add a append method working on Object? Maybe I have read the diff wrong, sorry. Anyway, some hints for writing method in DGM. First, you don't really have to use instanceof there. make two append methods, one for Writable and one for Object. Then, if you want to keep append(File,Object), you should keep the leftshift too. Maybe even adding a leftShift(File,Writable) would be good. ah, one more.. shouldn't we close the File in case of an Exception? Hmm, we should really remove all instanceof instructions in DGM where possible.
Hide
Permalink
John Wilson added a comment -

Actually I think

InvokerHelper.write(writer, text)

is better.

The type of writer should be Object. This means that any MetaClass magic gets invoked without the DGM method needing to worry about things.

Blackdreag's point about exceptions is well made.

Show
John Wilson added a comment - Actually I think InvokerHelper.write(writer, text) is better. The type of writer should be Object. This means that any MetaClass magic gets invoked without the DGM method needing to worry about things. Blackdreag's point about exceptions is well made.
Hide
Permalink
Joachim Baumann added a comment -

Ok, here's the next version.

Regarding the exceptions: I've tried to model the methods after the other methods I've seen. Should I then simply add the exception handling code to all other methods where exceptions can be thrown and a resource is to be closed regardless of that?

And I've changed the diff tool configuration to produce a unified diff.

Btw, thanks for the corrections

Show
Joachim Baumann added a comment - Ok, here's the next version. Regarding the exceptions: I've tried to model the methods after the other methods I've seen. Should I then simply add the exception handling code to all other methods where exceptions can be thrown and a resource is to be closed regardless of that? And I've changed the diff tool configuration to produce a unified diff. Btw, thanks for the corrections
Hide
Permalink
Joachim Baumann added a comment -

Another small correction. Now the exception thrown when closing a writer is logged.

Show
Joachim Baumann added a comment - Another small correction. Now the exception thrown when closing a writer is logged.
Hide
Permalink
John Wilson added a comment -

Thanks for the patch

Could you provide us with a unit test which excecises your changes, please?

Show
John Wilson added a comment - Thanks for the patch Could you provide us with a unit test which excecises your changes, please?
Hide
Permalink
blackdrag blackdrag added a comment -

Now I can read the diff, thx

About the exceptions, yes, I think so, but please in another issue. Make a new one. It is not good to have mayn issues discussed and solved in just one issue.

After a test case is added we cann put this into the codebase. Thx, very much Joachim

Show
blackdrag blackdrag added a comment - Now I can read the diff, thx About the exceptions, yes, I think so, but please in another issue. Make a new one. It is not good to have mayn issues discussed and solved in just one issue. After a test case is added we cann put this into the codebase. Thx, very much Joachim
Hide
Permalink
Joachim Baumann added a comment -

The test cases are next on my list. They'll be most probably coming begin of next week.

I agree with solving only one problem in an issue. For the other exceptions, I'll open another JIRA issue.

Show
Joachim Baumann added a comment - The test cases are next on my list. They'll be most probably coming begin of next week. I agree with solving only one problem in an issue. For the other exceptions, I'll open another JIRA issue.
Hide
Permalink
Joachim Baumann added a comment -

Here are the tests for GROOVY-1362. To be sure I test not only Writables, but Strings and Object.toString() as well.
I'm only testing positives, no negatives, since I assume that the underlying JDK is reasonably well-tested. Thus, if the mapping works, the rest should work as well.

I placed the test in /groovy-core/src/test/groovy/FileAppendTest.groovy.

Cheers, Joe

Show
Joachim Baumann added a comment - Here are the tests for GROOVY-1362. To be sure I test not only Writables, but Strings and Object.toString() as well. I'm only testing positives, no negatives, since I assume that the underlying JDK is reasonably well-tested. Thus, if the mapping works, the rest should work as well. I placed the test in /groovy-core/src/test/groovy/FileAppendTest.groovy. Cheers, Joe
Hide
Permalink
John Wilson added a comment -

Applied patch - Thanks!

Show
John Wilson added a comment - Applied patch - Thanks!
Hide
Permalink
John Wilson added a comment -

I have commited a small change to this patch to slighly simplyfy the processing. As we use Invoker.write() we don't have to distinguish between String, Object and Writable. Also the form of append which takes an encoding needs to close the Writer if there is an exception.

Show
John Wilson added a comment - I have commited a small change to this patch to slighly simplyfy the processing. As we use Invoker.write() we don't have to distinguish between String, Object and Writable. Also the form of append which takes an encoding needs to close the Writer if there is an exception.

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Codehaus. Try JIRA - bug tracking software for your team.