garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2372214794
##########
src/test/java/org/apache/commons/io/build/CharSequenceOriginTest.java:
##########
@@ -62,13 +62,6 @@ void testGetFile() {
assertThrows(UnsupportedOperationException.class, super::testGetFile);
}
- @Override
- @Test
- void testGetOutputStream() {
Review Comment:
Why was this method deleted and then added again? If you keep the methods in
AB order, this shouln't happen.
##########
src/test/java/org/apache/commons/io/build/OutputStreamOriginTest.java:
##########
@@ -89,13 +90,6 @@ void testGetFile() {
assertThrows(UnsupportedOperationException.class, super::testGetFile);
}
- @Override
Review Comment:
This method doesn't need to be moved; it makes the PR larger for no reason.
##########
src/test/java/org/apache/commons/io/build/InputStreamOriginTest.java:
##########
@@ -58,13 +59,6 @@ void testGetFile() {
assertThrows(UnsupportedOperationException.class, super::testGetFile);
}
- @Override
Review Comment:
This method doesn't need to be moved; it makes the PR larger for no reason.
##########
src/test/java/org/apache/commons/io/build/ReaderOriginTest.java:
##########
@@ -56,13 +57,6 @@ void testGetFile() {
assertThrows(UnsupportedOperationException.class, super::testGetFile);
}
- @Override
Review Comment:
This method doesn't need to be moved; it makes the PR larger for no reason.
##########
src/test/java/org/apache/commons/io/build/WriterStreamOriginTest.java:
##########
@@ -89,13 +90,6 @@ void testGetFile() {
assertThrows(UnsupportedOperationException.class, super::testGetFile);
}
- @Override
Review Comment:
This method doesn't need to be moved; it makes the PR larger for no reason.
##########
src/test/java/org/apache/commons/io/channels/ByteArraySeekableByteChannelCompressTest.java:
##########
@@ -62,7 +66,7 @@ void testCloseIsIdempotent() throws Exception {
@ParameterizedTest
@ValueSource(ints = { 0, 1, 2, 3, 4, 5, 6 })
void testReadingFromAPositionAfterEndReturnsEOF(final int size) throws
Exception {
- try (SeekableByteChannel c = new ByteArraySeekableByteChannel(size)) {
+ try (SeekableByteChannel c = ByteArraySeekableByteChannel.wrap(new
byte[size])) {
Review Comment:
Sure, but I am talking about the _all_ the tests that read from a byte array
like the ones below. I should have made that clearer in my initial comment. For
example, this is _not_ the pattern we want:
```diff
@Test
void testShouldReadContentsProperly() throws IOException {
- try (ByteArraySeekableByteChannel c = new
ByteArraySeekableByteChannel(testData)) {
+ try (ByteArraySeekableByteChannel c =
ByteArraySeekableByteChannel.wrap(cloneTestData())) {
```
A read test should not affect the underlying byte array so you want to give
it the raw array.
If we want to give it a copy, then maybe the PR should also make sure the
copy has not been altered?
##########
src/test/java/org/apache/commons/io/build/CharSequenceOriginTest.java:
##########
@@ -109,11 +123,4 @@ void testGetReaderIgnoreCharsetNull() throws IOException {
}
}
- @Override
Review Comment:
It's confusing to see a method deleted and added again.
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a new channel backed directly by the given byte array.
+ *
+ * <p>The channel initially contains the full contents of the array, with
its
+ * size set to {@code bytes.length} and its position set to {@code 0}.</p>
+ *
+ * <p>Reads and writes operate on the shared array.
+ * If a write operation extends beyond the current capacity, the channel
will
+ * automatically allocate a larger backing array and copy the existing
contents.</p>
+ *
+ * @param bytes The byte array to wrap, must not be {@code null}
+ * @return A new channel that uses the given array as its initial backing
store
+ * @throws NullPointerException If {@code bytes} is {@code null}
+ * @see #array()
+ * @see java.io.ByteArrayInputStream#ByteArrayInputStream(byte[])
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
Review Comment:
If you "Resolve a Conversation" in GH after answering a question, the person
asking the question doesn't see the answer! It's hidden behind a "Show
resolved" button. I think it makes sense for the person asking the question to
do the "Resolve". Now I have to open every "Show resolved" to see if a question
was truly resolved or not.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
