Skip to content

Replace CloseShield* constructors with factory methods #173

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.

Sign up for GitHub

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

Merged
garydgregory merged 3 commits into from
Dec 10, 2020
Merged

Replace CloseShield* constructors with factory methods #173

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
56af9be
Replace CloseShield* constructors with factory methods
robtimus Dec 6, 2020
d89454e
Renamed CloseShield*.dontClose to wrap
robtimus Dec 7, 2020
8bdc513
Changed parameter names and tags as requested
robtimus Dec 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Replace CloseShield* constructors with factory methods
  • Loading branch information
@robtimus
robtimus committed Dec 7, 2020
commit 56af9bec67e01255ff1ea26e194c1918e2bcbb59
16 changes: 16 additions & 0 deletions src/main/java/org/apache/commons/io/input/CloseShieldInputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ public class CloseShieldInputStream extends ProxyInputStream {
* closed.
*
* @param in underlying input stream
* @deprecated Using this constructor prevents IDEs from warning if
* the underlying input stream is never closed.
* Use {@link #dontClose(InputStream)} instead.
*/
@Deprecated

@garydgregory garydgregory Dec 8, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't agree with deprecating these APIs, different versions of different IDEs raise different warnings depending on your preferred configuration of said IDE. Let's keep this PR simpler and lighter, we can always add deprecation later if it is truly warranted.

@robtimus robtimus Dec 8, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I feel that without deprecating the constructors, this PR adds no real value. People that currently use the constructors will keep doing so because they see no reason to change their code, and new users of the classes may only get confused if there are two ways to do the same thing. They'll probably end up using the constructors too because that's a character shorter (new vs .wrap).

My thought was that deprecating the constructors will make current users think about whether or not they have closed the underlying streams, even if their IDEs don't warn them. New users will chose the wrapper method because the alternative is deprecated.

The only downside to deprecating the constructors that I can see is that current users that don't have their IDEs setup to warn about unclosed streams will get warnings where there were none first, but as I said, perhaps they will think about whether or not their streams are properly closed somewhere else.

public CloseShieldInputStream(final InputStream in) {
super(in);
}
Expand All @@ -50,4 +54,16 @@ public void close() {
in = ClosedInputStream.CLOSED_INPUT_STREAM;
}

/**
* Creates a proxy that shields the given input stream from being
* closed.
*
* @param in underlying input stream
Comment thread
robtimus marked this conversation as resolved.
Outdated
* @return the created proxy
* @since 2.9.0
*/
public static CloseShieldInputStream dontClose(final InputStream in) {
return new CloseShieldInputStream(in);
}

}
16 changes: 16 additions & 0 deletions src/main/java/org/apache/commons/io/input/CloseShieldReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ public class CloseShieldReader extends ProxyReader {
* closed.
*
* @param in underlying reader
* @deprecated Using this constructor prevents IDEs from warning if
* the underlying reader is never closed.
* Use {@link #dontClose(Reader)} instead.
*/
@Deprecated
public CloseShieldReader(final Reader in) {
super(in);
}
Expand All @@ -50,4 +54,16 @@ public void close() {
in = ClosedReader.CLOSED_READER;
}

/**
* Creates a proxy that shields the given reader from being
* closed.
*
* @param in underlying reader
* @return the created proxy
* @since 2.9.0
*/
public static CloseShieldReader dontClose(final Reader in) {
return new CloseShieldReader(in);
}

}
16 changes: 16 additions & 0 deletions src/main/java/org/apache/commons/io/output/CloseShieldOutputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ public class CloseShieldOutputStream extends ProxyOutputStream {
* closed.
*
* @param out underlying output stream
* @deprecated Using this constructor prevents IDEs from warning if
* the underlying output stream is never closed.
* Use {@link #dontClose(OutputStream)} instead.
*/
@Deprecated
public CloseShieldOutputStream(final OutputStream out) {
super(out);
}
Expand All @@ -50,4 +54,16 @@ public void close() {
out = ClosedOutputStream.CLOSED_OUTPUT_STREAM;
}

/**
* Creates a proxy that shields the given output stream from being
* closed.
*
* @param out underlying output stream
* @return the created proxy
* @since 2.9.0
*/
public static CloseShieldOutputStream dontClose(final OutputStream out) {
return new CloseShieldOutputStream(out);
}

}
15 changes: 15 additions & 0 deletions src/main/java/org/apache/commons/io/output/CloseShieldWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,11 @@ public class CloseShieldWriter extends ProxyWriter {
* Creates a proxy that shields the given writer from being closed.
*
* @param out underlying writer
* @deprecated Using this constructor prevents IDEs from warning if
* the underlying writer is never closed.
* Use {@link #dontClose(Writer)} instead.
*/
@Deprecated
public CloseShieldWriter(final Writer out) {
super(out);
}
Expand All @@ -47,4 +51,15 @@ public void close() {
out = ClosedWriter.CLOSED_WRITER;
}

/**
* Creates a proxy that shields the given writer from being closed.
*
* @param out underlying writer
* @return the created proxy
* @since 2.9.0
*/
public static CloseShieldWriter dontClose(final Writer out) {
return new CloseShieldWriter(out);
}

}
3 changes: 2 additions & 1 deletion src/test/java/org/apache/commons/io/input/CloseShieldInputStreamTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package org.apache.commons.io.input;

import static org.apache.commons.io.input.CloseShieldInputStream.dontClose;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;

Expand Down Expand Up @@ -48,7 +49,7 @@ public void close() {
closed = true;
}
};
shielded = new CloseShieldInputStream(original);
shielded = dontClose(original);
closed = false;
}

Expand Down
3 changes: 2 additions & 1 deletion src/test/java/org/apache/commons/io/input/CloseShieldReaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package org.apache.commons.io.input;

import static org.apache.commons.io.input.CloseShieldReader.dontClose;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.spy;
Expand All @@ -42,7 +43,7 @@ public class CloseShieldReaderTest {
public void setUp() {
data = "xyz";
original = spy(new CharSequenceReader(data));
shielded = new CloseShieldReader(original);
shielded = dontClose(original);
}

@Test
Expand Down
12 changes: 4 additions & 8 deletions src/test/java/org/apache/commons/io/output/CloseShieldOutputStreamTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@
*/
package org.apache.commons.io.output;

import static org.apache.commons.io.output.CloseShieldOutputStream.dontClose;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.fail;
import static org.junit.jupiter.api.Assertions.assertThrows;

import java.io.IOException;
import java.io.OutputStream;
Expand All @@ -45,20 +46,15 @@ public void close() {
closed = true;
}
};
shielded = new CloseShieldOutputStream(original);
shielded = dontClose(original);
closed = false;
}

@Test
public void testClose() throws IOException {
shielded.close();
assertFalse(closed, "closed");
try {
shielded.write('x');
fail("write(b)");
} catch (final IOException ignore) {
// expected
}
assertThrows(IOException.class, () -> shielded.write('x'), "write(b)");
original.write('y');
assertEquals(1, original.size());
assertEquals('y', original.toByteArray()[0]);
Expand Down
12 changes: 4 additions & 8 deletions src/test/java/org/apache/commons/io/output/CloseShieldWriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@
*/
package org.apache.commons.io.output;

import static org.apache.commons.io.output.CloseShieldWriter.dontClose;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.fail;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;
Expand All @@ -40,19 +41,14 @@ public class CloseShieldWriterTest {
@BeforeEach
public void setUp() {
original = spy(new StringBuilderWriter());
shielded = new CloseShieldWriter(original);
shielded = dontClose(original);
}

@Test
public void testClose() throws IOException {
shielded.close();
verify(original, never()).close();
try {
shielded.write('x');
fail("write(c)");
} catch (final IOException ignore) {
// expected
}
assertThrows(IOException.class, () -> shielded.write('x'), "write(c)");
Comment thread
robtimus marked this conversation as resolved.
original.write('y');
assertEquals(1, original.getBuilder().length());
assertEquals('y', original.toString().charAt(0));
Expand Down