Skip to content

IOUtils clean up javadoc#299

Closed
wodencafe wants to merge 1 commit intoapache:masterfrom
wodencafe:IOUtils-cleanup
Closed

IOUtils clean up javadoc#299
wodencafe wants to merge 1 commit intoapache:masterfrom
wodencafe:IOUtils-cleanup

Conversation

@wodencafe
Copy link
Contributor

@wodencafe wodencafe commented Nov 5, 2021

This PR removes some redundant methods from IOUtils, specifically those which use String and StringBuffer, as the same methods that accept CharSequence as a parameter exist, and it is the interface that String and StringBuffer both implement.
There are some other minor code and documentation tweaks.

EDIT: This PR now just updates the javadoc of IOUtils to be more accurate and tidier.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

This would need a JIRA issue, and since public methods are being removed, I think we will need to bump it to the next major release (or just deprecate instead of removing methods). Thanks!

@wodencafe
Copy link
Contributor Author

wodencafe commented Nov 5, 2021

Hi @kinow , thank you for taking the time to review this pull request. I have a question for you about the deprecated / removed methods. Since CharSequence is the interface that is implemented by the types that the (removed in the PR) methods accepted as first parameter (String, StringBuffer), is it still necessary to retain those overloaded methods, since the existing methods that accept CharSequence are using the same logic?

@kinow
Copy link
Member

kinow commented Nov 5, 2021

Hi @kinow , thank you for taking the time to review this pull request. I have a question for you about the deprecated / removed methods. Since CharSequence is the interface that is implemented by the types that the (removed in the PR) methods accepted as first parameter (String, StringBuffer), is it still necessary to retain those overloaded methods, since the existing methods that accept CharSequence are using the same logic?

Hi @wodencafe the issue is backward compatibility.

If we remove public methods (or modify the code in a way that breaks binary backward compatibility 12 or sometimes behavioral backward compatibility 34) we need to think about the users of the API first.

We could have users of Commons IO that are using those methods in their code. So if we remove these methods, users would expect a major version release so they are aware that there are API changes. Releasing major versions can be inconvenient to users too, as even those that were not affected will have to either review the changes or simply upgrade their code. For that reason, we accumulate changes that require major releases by deprecating unwanted code whenever possible.

I think your change makes sense, if users can rely on the parent type for String/StringBuffer. That would be less code to maintain I guess. We just may need to delay reviewing and merging it until we are ready for a major release 👍

Hope that makes sense.

Thanks!
Bruno

Footnotes

  1. https://www.oracle.com/java/technologies/compatibility.html#binary

  2. https://en.wikipedia.org/wiki/Binary-code_compatibility

  3. https://www.oracle.com/java/technologies/compatibility.html#behavioral

  4. https://docs.microsoft.com/en-us/dotnet/core/compatibility/categories#behavioral-change

@garydgregory
Copy link
Member

-1 this breaks binary compatibility. There is no need to do that at this time.

We can consider dropping all deprecations in 3.0, and there are no plans for that yet.

@wodencafe
Copy link
Contributor Author

Ah thank you for providing links and explaining @kinow and @garydgregory , I will put the methods back and update the pull request accordingly.

@kinow
Copy link
Member

kinow commented Nov 6, 2021

Brilliant @wodencafe ! I haven't looked at CI, but it appears to be failing. We need to confirm if it is something with this PR or if it is failing on master too.

@garydgregory
Copy link
Member

Binary compatibility is still broken. See the builds referred to from this PR.

@garydgregory
Copy link
Member

Let's close this PR unless there are changes that won't break BC.

@wodencafe
Copy link
Contributor Author

Let's close this PR unless there are changes that won't break BC.

Hi @garydgregory and @kinow , I have an updated version of this branch locally that only changes the Javadocs to appropriately match the contract / functionality of the IOUtils methods, and also some misc. formatting / fixes to the existing Javadoc itself. I should be able to push the changes tonight / tomorrow.

Thank you for the review and feedback.

@wodencafe wodencafe changed the title IOUtils clean up redundant methods. Docs and misc fixes. IOUtils clean up javadoc Nov 10, 2021
@wodencafe
Copy link
Contributor Author

Hello again @kinow and @garydgregory , I have updated this Pull Request to tidy up the javadoc in IOUtils.

@wodencafe wodencafe force-pushed the IOUtils-cleanup branch 2 times, most recently from 12b481e to b56ebc5 Compare November 10, 2021 05:25
@garydgregory
Copy link
Member

Lots of changes here, TY. I'll try to go through them all this weekend.

@garydgregory
Copy link
Member

HI @wodencafe
It's quite hard to review such a giant changeset that claims to only be about Javadoc but also changes the code in contentEqualsIgnoreEOL, so let's drop code changes, then I can just review Javadoc. TY.

* @since 2.9.0
*/
public static byte[] byteArray(final int size) {
return new byte[size];
}

/**
* Returns a new char array of size {@link #DEFAULT_BUFFER_SIZE}.
* Returns a new {@code char[]} array of size {@link #DEFAULT_BUFFER_SIZE}.
Copy link
Member

@garydgregory garydgregory Apr 17, 2023

Choose a reason for hiding this comment

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

This is no good IMO: "a a new char array" is clear and correct. "a new {@code char} array" would also be correct as would "a new {@code char[]}" BUT "a "new {@code char[]} array" reads like an array of array. So let's pass on this.

@garydgregory
Copy link
Member

Closing, no feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants