[GitHub] [commons-io] garydgregory commented on a change in pull request #173: Replace CloseShield* constructors with factory methods

[GitBox <gi...@apache.org>]

garydgregory commented on a change in pull request #173:
URL: https://github.com/apache/commons-io/pull/173#discussion_r538433221



##########
File path: src/main/java/org/apache/commons/io/input/CloseShieldInputStream.java
##########
@@ -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

Review comment:
       `@param in underlying input stream` -> `@param inputStream the input stream to wrap`

##########
File path: src/main/java/org/apache/commons/io/input/CloseShieldReader.java
##########
@@ -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 wrap(final Reader in) {

Review comment:
       as see other Javadoc comments, in this case `in` -> `reader`

##########
File path: src/main/java/org/apache/commons/io/input/CloseShieldInputStream.java
##########
@@ -35,7 +35,11 @@
      * closed.
      *
      * @param in underlying input stream
+     * @deprecated Using this constructor prevents IDEs from warning if
+     *             the underlying input stream is never closed.
+     *             Use {@link #wrap(InputStream)} instead.
      */
+    @Deprecated

Review comment:
       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.
   

##########
File path: src/test/java/org/apache/commons/io/output/CloseShieldWriterTest.java
##########
@@ -40,19 +40,14 @@
     @BeforeEach
     public void setUp() {
         original = spy(new StringBuilderWriter());
-        shielded = new CloseShieldWriter(original);
+        shielded = CloseShieldWriter.wrap(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)");

Review comment:
       Nice use of a new JUnit 5 API! 👍 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org