Better docs in IOUtils and IOUtils.byteArray(int size) by ArdenL-Liu · Pull Request #374 · apache/commons-io

ArdenL-Liu · 2022-08-24T01:45:37Z

IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.

…ize is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.

kinow · 2022-08-24T01:55:18Z

src/test/java/org/apache/commons/io/IOUtilsTest.java

+            bytes = IOUtils.byteArray(size);
+        }catch (Exception e) {
+            assertEquals(e.getClass().getName(), IllegalArgumentException.class.getName());
+        }


I think instead it could use JUnit's @Test(expected=IllegalArgumentException.class), or assertThrows.

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.

ok, I changed IOUtilsTest.testByteArrayWithIllegalSize() with @ParameterizedTest.
you can review the code.

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.

Hi garydgregory,
Thank you for your good suggestion, I updated the doc and changed the unittest, you can review it again.

In order to improve my work efficiency，I will study github workflow in the next week.

kinow · 2022-08-24T01:58:32Z

src/main/java/org/apache/commons/io/IOUtils.java

     * @param size array size.
     * @return a new byte array of the given size.
+     *
+     * @exception  IllegalArgumentException  If {@code size <= 0}


@throws instead of @exception

Thx, update it with @throws

kinow · 2022-08-24T02:14:40Z

src/main/java/org/apache/commons/io/IOUtils.java

     * Instances should NOT be constructed in standard programming.
+     * @deprecated Will be private in 3.0.
     */
+    @Deprecated


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.

Thx, I will rollback it

Can you remove this deprecation as @kinow requested?

Wait... this deprecation is fine. It's slightly out of scope for the PR but it's fine.

…Test

pjfanning · 2022-08-25T16:54:58Z

src/main/java/org/apache/commons/io/FileUtils.java

     * Instances should NOT be constructed in standard programming.
-     * @deprecated Will be private in 3.0.
     */
-    @Deprecated


Should this @Deprecated be removed? There was a comment to remove a different @Deprecated but this one should be kept.

rollback it

-1: Don't remove this tag,

this is not resolved - you have still removed a comment that was in the code before - can you put it back?

The @deprecated tag should NOT be removed.

Hi All,
sorry for my error operation, I fixed the doc and add @deprecated tag about FileUtils.

public FileUtils() { //NOSONAR

garydgregory · 2022-08-26T11:52:04Z

src/main/java/org/apache/commons/io/IOUtils.java


    /**
     * Returns a new byte array of the given size.
+     * Throws java.lang.NegativeArraySizeException if the size is negative.


This is in the wrong place, use a throws tag.

Hi garydgregory,
I fixed it with @throws, you can review it again.

garydgregory · 2022-08-26T11:57:18Z

src/test/java/org/apache/commons/io/IOUtilsTest.java

+    @Test
+    public void testByteArrayWithNegativeSize() {
+        int size = -1;
+        assertThrows(NegativeArraySizeException.class,() -> {


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?

Hi garydgregory,
from your comments, I checked the code and fixed again, you can review it again.
Thanks.

src/test/java/org/apache/commons/io/IOUtilsTest.java

garydgregory

Hello @ArdenL-Liu
Thank you for the PR, please see my comments.

codecov-commenter · 2022-08-26T12:16:45Z

Codecov Report

Merging #374 (00a1e25) into master (e5683ee) will decrease coverage by 0.08%.
The diff coverage is n/a.

@@             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	Coverage Δ
src/main/java/org/apache/commons/io/FileUtils.java	`94.42% <ø> (ø)`
src/main/java/org/apache/commons/io/IOUtils.java	`85.82% <ø> (ø)`
.../apache/commons/io/input/ReadAheadInputStream.java	`68.53% <0.00%> (-5.06%)`	⬇️
.../main/java/org/apache/commons/io/input/Tailer.java	`87.56% <0.00%> (+1.49%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

garydgregory · 2022-08-27T14:51:22Z

src/main/java/org/apache/commons/io/IOUtils.java

     *
     * @param size array size.
     * @return a new byte array of the given size.
+     * @throws NegativeArraySizeException if the size is negative.


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.

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).

garydgregory · 2022-08-28T16:22:54Z

src/test/java/org/apache/commons/io/IOUtilsTest.java

 */
 package org.apache.commons.io;

-import static org.junit.jupiter.api.Assertions.assertArrayEquals;


Remove noise in the PR: There is no need to update these imports.

garydgregory · 2022-08-28T16:23:17Z

src/test/java/org/apache/commons/io/IOUtilsTest.java

-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.*;


"*" imports are a no go

garydgregory · 2022-08-29T12:28:08Z

FYI: I've handled the Javadoc "java.lang" issue separately throughout most Apache Commons components.

garydgregory · 2022-08-31T19:26:14Z

@ArdenL-Liu
Please rebase on git master and see any comments that may be outstanding.

ArdenL-Liu · 2022-09-01T01:33:11Z

Hi garydgregory，
You're great. the comments updated in master line.

IOUtils.byteArray(int size) add the verification to assure that the s…

40d74ed

…ize is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.

kinow reviewed Aug 24, 2022

View reviewed changes

The constructure of IOUtils should be Deprecated as FileUtils

dfa1584

kinow requested changes Aug 24, 2022

View reviewed changes

ArdenL-Liu added 4 commits August 24, 2022 10:43

update with the suggestion

0562636

update with the suggestion

a80416b

update with the suggestion

6e1f92b

change IOUtilsTest.testByteArrayWithIllegalSize() with @Parameterized…

757b726

…Test

ArdenL-Liu requested a review from kinow August 24, 2022 03:45

pjfanning reviewed Aug 25, 2022

View reviewed changes

ArdenL-Liu added 2 commits August 26, 2022 09:02

@deprecated

8c1ce1e

public FileUtils() { //NOSONAR

update doc with the review suggestion.

00a1e25

garydgregory reviewed Aug 26, 2022

View reviewed changes

src/test/java/org/apache/commons/io/IOUtilsTest.java Outdated Show resolved Hide resolved

garydgregory requested changes Aug 26, 2022

View reviewed changes

ArdenL-Liu added 2 commits August 26, 2022 23:59

add tag :@throws NegativeArraySizeException if the size is negative.

d2358c2

rollback the doc of FileUtils.constructure

2c34de0

garydgregory reviewed Aug 27, 2022

View reviewed changes

garydgregory reviewed Aug 28, 2022

View reviewed changes

update imports of IOUtilsTest.java

9b76aa1

Merge branch 'apache:master' into master

02a0de5

ArdenL-Liu requested review from garydgregory and removed request for kinow September 1, 2022 02:00

ArdenL-Liu requested review from garydgregory and kinow and removed request for garydgregory and kinow September 2, 2022 01:00

ArdenL-Liu and others added 3 commits September 3, 2022 14:57

Merge branch 'apache:master' into master

8703813

Not so verbose

da42a86

Format tweak

50f233b

garydgregory changed the title ~~IOUtils.byteArray(int size) add the verification to assure that the size is legal(size > 0), the illegal(size <=0) should throw IllegalArgumentException.~~ Better docs in IOUtils and IOUtils.byteArray(int size) Sep 14, 2022

garydgregory merged commit d259b21 into apache:master Sep 14, 2022

asfgit pushed a commit that referenced this pull request Sep 14, 2022

Better docs in IOUtils and IOUtils.byteArray(int size) #374.

18ff1a5

Conversation

ArdenL-Liu commented Aug 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 26, 2022

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArdenL-Liu Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory commented Aug 29, 2022

Uh oh!

garydgregory commented Aug 31, 2022

Uh oh!

ArdenL-Liu commented Sep 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

garydgregory Aug 25, 2022 •

edited

Loading

garydgregory Aug 26, 2022 •

edited

Loading

ArdenL-Liu Aug 29, 2022 •

edited

Loading