You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/08/24 01:45:38 UTC
[GitHub] [commons-io] ArdenL-Liu opened a new pull request, #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
ArdenL-Liu opened a new pull request, #374:
URL: https://github.com/apache/commons-io/pull/374
IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r955968679
##########
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##########
@@ -79,6 +79,8 @@
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
Review Comment:
-1: Unused. Run `mvn` from the command line before pushing a PR to catch these issues.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] kinow commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r953250775
##########
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##########
@@ -1716,4 +1716,18 @@ public void testWriteLittleString() throws IOException {
}
}
+ @Test
+ public void testByteArrayWithIllegalSize() {
+ try {
+ int size = -1;
+ byte[] bytes = IOUtils.byteArray(size);
+
+ size = 0;
+ bytes = IOUtils.byteArray(size);
+ }catch (Exception e) {
+ assertEquals(e.getClass().getName(), IllegalArgumentException.class.getName());
+ }
Review Comment:
I think instead it could use JUnit's `@Test(expected=IllegalArgumentException.class)`, or `assertThrows`.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] ArdenL-Liu commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
ArdenL-Liu commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r956584181
##########
src/main/java/org/apache/commons/io/FileUtils.java:
##########
@@ -3537,9 +3537,7 @@ public static void writeStringToFile(final File file, final String data, final S
/**
* Instances should NOT be constructed in standard programming.
- * @deprecated Will be private in 3.0.
*/
- @Deprecated
Review Comment:
Hi All,
sorry for my error operation, I fixed the doc and add @deprecated tag about FileUtils.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] ArdenL-Liu commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
ArdenL-Liu commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r956840281
##########
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##########
@@ -60,25 +40,7 @@
import java.util.List;
import java.util.stream.Stream;
-import org.apache.commons.io.function.IOConsumer;
-import org.apache.commons.io.input.BrokenInputStream;
-import org.apache.commons.io.input.CircularInputStream;
-import org.apache.commons.io.input.NullInputStream;
-import org.apache.commons.io.input.NullReader;
-import org.apache.commons.io.input.StringInputStream;
-import org.apache.commons.io.output.AppendableWriter;
-import org.apache.commons.io.output.BrokenOutputStream;
-import org.apache.commons.io.output.CountingOutputStream;
-import org.apache.commons.io.output.NullOutputStream;
-import org.apache.commons.io.output.NullWriter;
-import org.apache.commons.io.output.StringBuilderWriter;
-import org.apache.commons.io.test.TestUtils;
-import org.apache.commons.io.test.ThrowOnCloseReader;
-import org.apache.commons.lang3.StringUtils;
-import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.Disabled;
-import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.io.TempDir;
+import static org.junit.jupiter.api.Assertions.*;
Review Comment:
fixed
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] ArdenL-Liu commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
ArdenL-Liu commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r955621214
##########
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##########
@@ -1716,4 +1716,18 @@ public void testWriteLittleString() throws IOException {
}
}
+ @Test
+ public void testByteArrayWithIllegalSize() {
+ try {
+ int size = -1;
+ byte[] bytes = IOUtils.byteArray(size);
+
+ size = 0;
+ bytes = IOUtils.byteArray(size);
+ }catch (Exception e) {
+ assertEquals(e.getClass().getName(), IllegalArgumentException.class.getName());
+ }
Review Comment:
Hi garydgregory,
Thank you for your good suggestion, I updated the doc and changed the unittest, you can review it again.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] pjfanning commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
pjfanning commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r956210410
##########
src/main/java/org/apache/commons/io/FileUtils.java:
##########
@@ -3537,9 +3537,7 @@ public static void writeStringToFile(final File file, final String data, final S
/**
* Instances should NOT be constructed in standard programming.
- * @deprecated Will be private in 3.0.
*/
- @Deprecated
Review Comment:
this is not resolved - you have still removed a comment that was in the code before - can you put it back?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r955968481
##########
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##########
@@ -1716,4 +1718,12 @@ public void testWriteLittleString() throws IOException {
}
}
+ @Test
+ public void testByteArrayWithNegativeSize() {
+ int size = -1;
+ assertThrows(NegativeArraySizeException.class,() -> {
Review Comment:
You can just say `assertThrows(NegativeArraySizeException.class,() -> IOUtils.byteArray(-1));` but I am not even sure we should have this test since it does not improve our code coverage, it just test what the JVM does.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r956301439
##########
src/main/java/org/apache/commons/io/FileUtils.java:
##########
@@ -3537,9 +3537,7 @@ public static void writeStringToFile(final File file, final String data, final S
/**
* Instances should NOT be constructed in standard programming.
- * @deprecated Will be private in 3.0.
*/
- @Deprecated
Review Comment:
The `@deprecated` tag should NOT be removed.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] ArdenL-Liu commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
ArdenL-Liu commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r956844679
##########
src/main/java/org/apache/commons/io/IOUtils.java:
##########
@@ -346,6 +346,7 @@ public static byte[] byteArray() {
*
* @param size array size.
* @return a new byte array of the given size.
+ * @throws NegativeArraySizeException if the size is negative.
Review Comment:
As yours, this is why I want to add throws NegativeArraySizeException tag to this method, In fact, in order to help users to use and understand these methods, I think we should add it to the doc of all the methods like it.
Of course , we can add the parameters validation to the head position of the method's logic. some methods do it, such as : read(...., int), skip(... ,int).
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r956751783
##########
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##########
@@ -16,40 +16,20 @@
*/
package org.apache.commons.io;
-import static org.junit.jupiter.api.Assertions.assertArrayEquals;
Review Comment:
Remove noise in the PR: There is no need to update these imports.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] kinow commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r953252075
##########
src/main/java/org/apache/commons/io/IOUtils.java:
##########
@@ -346,9 +346,15 @@ public static byte[] byteArray() {
*
* @param size array size.
* @return a new byte array of the given size.
+ *
+ * @exception IllegalArgumentException If {@code size <= 0}
Review Comment:
`@throws` instead of `@exception`
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r956751851
##########
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##########
@@ -60,25 +40,7 @@
import java.util.List;
import java.util.stream.Stream;
-import org.apache.commons.io.function.IOConsumer;
-import org.apache.commons.io.input.BrokenInputStream;
-import org.apache.commons.io.input.CircularInputStream;
-import org.apache.commons.io.input.NullInputStream;
-import org.apache.commons.io.input.NullReader;
-import org.apache.commons.io.input.StringInputStream;
-import org.apache.commons.io.output.AppendableWriter;
-import org.apache.commons.io.output.BrokenOutputStream;
-import org.apache.commons.io.output.CountingOutputStream;
-import org.apache.commons.io.output.NullOutputStream;
-import org.apache.commons.io.output.NullWriter;
-import org.apache.commons.io.output.StringBuilderWriter;
-import org.apache.commons.io.test.TestUtils;
-import org.apache.commons.io.test.ThrowOnCloseReader;
-import org.apache.commons.lang3.StringUtils;
-import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.Disabled;
-import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.io.TempDir;
+import static org.junit.jupiter.api.Assertions.*;
Review Comment:
"*" imports are a no go
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] ArdenL-Liu commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
ArdenL-Liu commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r956209112
##########
src/main/java/org/apache/commons/io/IOUtils.java:
##########
@@ -341,6 +341,7 @@ public static byte[] byteArray() {
/**
* Returns a new byte array of the given size.
+ * Throws java.lang.NegativeArraySizeException if the size is negative.
Review Comment:
Hi garydgregory,
I fixed it with @throws, you can review it again.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #374:
URL: https://github.com/apache/commons-io/pull/374#issuecomment-1233335045
@ArdenL-Liu
Please **rebase** on git master and see any comments that may be outstanding.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] ArdenL-Liu commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
ArdenL-Liu commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r956844679
##########
src/main/java/org/apache/commons/io/IOUtils.java:
##########
@@ -346,6 +346,7 @@ public static byte[] byteArray() {
*
* @param size array size.
* @return a new byte array of the given size.
+ * @throws NegativeArraySizeException if the size is negative.
Review Comment:
As yours, this is why I want to add throws NegativeArraySizeException tag to this method, In fact, in order to help users to use and understand these methods, I think we should add it to the doc of all methods like it.
Of course , we can add the parameters validation to the head position of the method's logic. some methods has done it, such as : read(...., int), skip(... ,int).
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r955968481
##########
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##########
@@ -1716,4 +1718,12 @@ public void testWriteLittleString() throws IOException {
}
}
+ @Test
+ public void testByteArrayWithNegativeSize() {
+ int size = -1;
+ assertThrows(NegativeArraySizeException.class,() -> {
Review Comment:
You can just say `assertThrows(NegativeArraySizeException.class,() -> IOUtils.byteArray(-1));` but I am not even sure we should have this test since it does not improve our code coverage, it just test what the JVM does. @kinow , any thoughts?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r955968679
##########
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##########
@@ -79,6 +79,8 @@
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
Review Comment:
-1: Unused
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] codecov-commenter commented on pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #374:
URL: https://github.com/apache/commons-io/pull/374#issuecomment-1228417331
# [Codecov](https://codecov.io/gh/apache/commons-io/pull/374?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#374](https://codecov.io/gh/apache/commons-io/pull/374?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (00a1e25) into [master](https://codecov.io/gh/apache/commons-io/commit/e5683eec11d4b81acc50e0cd17902f23a789c6fa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e5683ee) will **decrease** coverage by `0.08%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #374 +/- ##
============================================
- Coverage 85.71% 85.63% -0.09%
Complexity 3101 3101
============================================
Files 204 204
Lines 7365 7365
Branches 904 904
============================================
- Hits 6313 6307 -6
- Misses 804 811 +7
+ Partials 248 247 -1
```
| [Impacted Files](https://codecov.io/gh/apache/commons-io/pull/374?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [src/main/java/org/apache/commons/io/FileUtils.java](https://codecov.io/gh/apache/commons-io/pull/374/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvaW8vRmlsZVV0aWxzLmphdmE=) | `94.42% <ø> (ø)` | |
| [src/main/java/org/apache/commons/io/IOUtils.java](https://codecov.io/gh/apache/commons-io/pull/374/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvaW8vSU9VdGlscy5qYXZh) | `85.82% <ø> (ø)` | |
| [.../apache/commons/io/input/ReadAheadInputStream.java](https://codecov.io/gh/apache/commons-io/pull/374/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvaW8vaW5wdXQvUmVhZEFoZWFkSW5wdXRTdHJlYW0uamF2YQ==) | `68.53% <0.00%> (-5.06%)` | :arrow_down: |
| [.../main/java/org/apache/commons/io/input/Tailer.java](https://codecov.io/gh/apache/commons-io/pull/374/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvaW8vaW5wdXQvVGFpbGVyLmphdmE=) | `87.56% <0.00%> (+1.49%)` | :arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] kinow commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r953258434
##########
src/main/java/org/apache/commons/io/IOUtils.java:
##########
@@ -3724,7 +3724,9 @@ public static Writer writer(final Appendable appendable) {
/**
* Instances should NOT be constructed in standard programming.
+ * @deprecated Will be private in 3.0.
*/
+ @Deprecated
Review Comment:
This is doing more than what's described in the PR title. Maybe worth moving to a separate PR to make it easier to track in the changelog.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r956590623
##########
src/main/java/org/apache/commons/io/IOUtils.java:
##########
@@ -346,6 +346,7 @@ public static byte[] byteArray() {
*
* @param size array size.
* @return a new byte array of the given size.
+ * @throws NegativeArraySizeException if the size is negative.
Review Comment:
This PR seems ok to me now except for this addition. We do not document this type of JVM exception anywhere else IIRC. If we were to doc this, then it would make sense to doc all callers as well, then this would touch almost everything which would not help anyone IMO.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r955334081
##########
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##########
@@ -1716,4 +1716,18 @@ public void testWriteLittleString() throws IOException {
}
}
+ @Test
+ public void testByteArrayWithIllegalSize() {
+ try {
+ int size = -1;
+ byte[] bytes = IOUtils.byteArray(size);
+
+ size = 0;
+ bytes = IOUtils.byteArray(size);
+ }catch (Exception e) {
+ assertEquals(e.getClass().getName(), IllegalArgumentException.class.getName());
+ }
Review Comment:
Why would we trade a more vague exception when the JVM throws a more precise and quite specific exception? Maybe this issue can be simplified with better Javadoc? This PR would also introduce an inconsistency since other places would throw the JVM exception.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] ArdenL-Liu commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
ArdenL-Liu commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r953266013
##########
src/main/java/org/apache/commons/io/IOUtils.java:
##########
@@ -3724,7 +3724,9 @@ public static Writer writer(final Appendable appendable) {
/**
* Instances should NOT be constructed in standard programming.
+ * @deprecated Will be private in 3.0.
*/
+ @Deprecated
Review Comment:
Thx, I will rollback it
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] ArdenL-Liu commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
ArdenL-Liu commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r953272166
##########
src/main/java/org/apache/commons/io/IOUtils.java:
##########
@@ -346,9 +346,15 @@ public static byte[] byteArray() {
*
* @param size array size.
* @return a new byte array of the given size.
+ *
+ * @exception IllegalArgumentException If {@code size <= 0}
Review Comment:
Thx, update it with @throws
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r955334081
##########
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##########
@@ -1716,4 +1716,18 @@ public void testWriteLittleString() throws IOException {
}
}
+ @Test
+ public void testByteArrayWithIllegalSize() {
+ try {
+ int size = -1;
+ byte[] bytes = IOUtils.byteArray(size);
+
+ size = 0;
+ bytes = IOUtils.byteArray(size);
+ }catch (Exception e) {
+ assertEquals(e.getClass().getName(), IllegalArgumentException.class.getName());
+ }
Review Comment:
Why would we trade a more vague exception when the JVM throws a more precise and quite specific exception? Maybe this issue can be simplified with better Javadoc?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory merged pull request #374: Better docs in IOUtils and IOUtils.byteArray(int size)
Posted by GitBox <gi...@apache.org>.
garydgregory merged PR #374:
URL: https://github.com/apache/commons-io/pull/374
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] kinow commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r953251934
##########
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##########
@@ -1716,4 +1716,18 @@ public void testWriteLittleString() throws IOException {
}
}
+ @Test
+ public void testByteArrayWithIllegalSize() {
+ try {
+ int size = -1;
+ byte[] bytes = IOUtils.byteArray(size);
+
+ size = 0;
+ bytes = IOUtils.byteArray(size);
+ }catch (Exception e) {
+ assertEquals(e.getClass().getName(), IllegalArgumentException.class.getName());
+ }
Review Comment:
And I think when the first exception happens, the second is part of the test is never executed? Probably needs to be broken down into two separate tests, maybe with `@ParameterizedTest` too.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r955964557
##########
src/main/java/org/apache/commons/io/IOUtils.java:
##########
@@ -341,6 +341,7 @@ public static byte[] byteArray() {
/**
* Returns a new byte array of the given size.
+ * Throws java.lang.NegativeArraySizeException if the size is negative.
Review Comment:
This is in the wrong place, use a throws tag.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] pjfanning commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
pjfanning commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r956211162
##########
src/main/java/org/apache/commons/io/IOUtils.java:
##########
@@ -3724,7 +3724,9 @@ public static Writer writer(final Appendable appendable) {
/**
* Instances should NOT be constructed in standard programming.
+ * @deprecated Will be private in 3.0.
*/
+ @Deprecated
Review Comment:
Can you remove this deprecation as @kinow requested?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] ArdenL-Liu commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
ArdenL-Liu commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r956206708
##########
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##########
@@ -1716,4 +1718,12 @@ public void testWriteLittleString() throws IOException {
}
}
+ @Test
+ public void testByteArrayWithNegativeSize() {
+ int size = -1;
+ assertThrows(NegativeArraySizeException.class,() -> {
Review Comment:
Hi garydgregory,
from your comments, I checked the code and fixed again, you can review it again.
Thanks.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] ArdenL-Liu commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
ArdenL-Liu commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r956585719
##########
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##########
@@ -1716,4 +1716,18 @@ public void testWriteLittleString() throws IOException {
}
}
+ @Test
+ public void testByteArrayWithIllegalSize() {
+ try {
+ int size = -1;
+ byte[] bytes = IOUtils.byteArray(size);
+
+ size = 0;
+ bytes = IOUtils.byteArray(size);
+ }catch (Exception e) {
+ assertEquals(e.getClass().getName(), IllegalArgumentException.class.getName());
+ }
Review Comment:
In order to improve my work efficiency，I will study github workflow in the next week.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] ArdenL-Liu commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
ArdenL-Liu commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r956844679
##########
src/main/java/org/apache/commons/io/IOUtils.java:
##########
@@ -346,6 +346,7 @@ public static byte[] byteArray() {
*
* @param size array size.
* @return a new byte array of the given size.
+ * @throws NegativeArraySizeException if the size is negative.
Review Comment:
As yours, this is why I want to add throws NegativeArraySizeException tag to this method, In fact, in order to help users to use and understand these methods, I think we should add it to the doc of all methods like it.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] ArdenL-Liu commented on pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
ArdenL-Liu commented on PR #374:
URL: https://github.com/apache/commons-io/pull/374#issuecomment-1233624861
Hi garydgregory，
thank you, I will rebase on git master.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #374:
URL: https://github.com/apache/commons-io/pull/374#issuecomment-1230219811
FYI: I've handled the Javadoc "java.lang" issue separately throughout most Apache Commons components.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r955334081
##########
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##########
@@ -1716,4 +1716,18 @@ public void testWriteLittleString() throws IOException {
}
}
+ @Test
+ public void testByteArrayWithIllegalSize() {
+ try {
+ int size = -1;
+ byte[] bytes = IOUtils.byteArray(size);
+
+ size = 0;
+ bytes = IOUtils.byteArray(size);
+ }catch (Exception e) {
+ assertEquals(e.getClass().getName(), IllegalArgumentException.class.getName());
+ }
Review Comment:
Why would we trade a more vague exception for the more precise and quite specific one already thrown by the VM? Maybe this issue can be simplified with better Javadoc?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] ArdenL-Liu commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
ArdenL-Liu commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r955544499
##########
src/main/java/org/apache/commons/io/FileUtils.java:
##########
@@ -3537,9 +3537,7 @@ public static void writeStringToFile(final File file, final String data, final S
/**
* Instances should NOT be constructed in standard programming.
- * @deprecated Will be private in 3.0.
*/
- @Deprecated
Review Comment:
rollback it
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r955964088
##########
src/main/java/org/apache/commons/io/FileUtils.java:
##########
@@ -3537,9 +3537,7 @@ public static void writeStringToFile(final File file, final String data, final S
/**
* Instances should NOT be constructed in standard programming.
- * @deprecated Will be private in 3.0.
*/
- @Deprecated
Review Comment:
-1: Don't remove this tag,
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] ArdenL-Liu commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
ArdenL-Liu commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r956844679
##########
src/main/java/org/apache/commons/io/IOUtils.java:
##########
@@ -346,6 +346,7 @@ public static byte[] byteArray() {
*
* @param size array size.
* @return a new byte array of the given size.
+ * @throws NegativeArraySizeException if the size is negative.
Review Comment:
As yours, this is why I want to add throws NegativeArraySizeException tag to this method, In fact, in order to help users to use and understand these methods, I think we should add it to the doc of all methods like it.
Of course , we can add the parameters validation to the head position of the method's logic. some methods do it, such as : read(...., int), skip(... ,int).
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] pjfanning commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
pjfanning commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r955213560
##########
src/main/java/org/apache/commons/io/FileUtils.java:
##########
@@ -3537,9 +3537,7 @@ public static void writeStringToFile(final File file, final String data, final S
/**
* Instances should NOT be constructed in standard programming.
- * @deprecated Will be private in 3.0.
*/
- @Deprecated
Review Comment:
Should this `@Deprecated` be removed? There was a comment to remove a different `@Deprecated` but this one should be kept.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] ArdenL-Liu commented on a diff in pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
ArdenL-Liu commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r953284699
##########
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##########
@@ -1716,4 +1716,18 @@ public void testWriteLittleString() throws IOException {
}
}
+ @Test
+ public void testByteArrayWithIllegalSize() {
+ try {
+ int size = -1;
+ byte[] bytes = IOUtils.byteArray(size);
+
+ size = 0;
+ bytes = IOUtils.byteArray(size);
+ }catch (Exception e) {
+ assertEquals(e.getClass().getName(), IllegalArgumentException.class.getName());
+ }
Review Comment:
ok, I changed IOUtilsTest.testByteArrayWithIllegalSize() with @ParameterizedTest.
you can review the code.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #374: Better docs in IOUtils and IOUtils.byteArray(int size)
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #374:
URL: https://github.com/apache/commons-io/pull/374#discussion_r970931957
##########
src/main/java/org/apache/commons/io/IOUtils.java:
##########
@@ -3724,7 +3724,9 @@ public static Writer writer(final Appendable appendable) {
/**
* Instances should NOT be constructed in standard programming.
+ * @deprecated Will be private in 3.0.
*/
+ @Deprecated
Review Comment:
Wait... this deprecation is fine. It's slightly out of scope for the PR but it's fine.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] ArdenL-Liu commented on pull request #374: IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.
Posted by GitBox <gi...@apache.org>.
ArdenL-Liu commented on PR #374:
URL: https://github.com/apache/commons-io/pull/374#issuecomment-1233629592
Hi garydgregory，
You're great. the comments updated in master line.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org