commons-io icon indicating copy to clipboard operation
commons-io copied to clipboard

IOUtils clean up javadoc

Open wodencafe opened this issue 4 years ago • 11 comments

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.

wodencafe avatar Nov 05 '21 15:11 wodencafe

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?

wodencafe avatar Nov 05 '21 21:11 wodencafe

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 ^1 or sometimes behavioral backward compatibility ^3) 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 :+1:

Hope that makes sense.

Thanks! Bruno

kinow avatar Nov 05 '21 22:11 kinow

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

garydgregory avatar Nov 05 '21 22:11 garydgregory

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

wodencafe avatar Nov 05 '21 23:11 wodencafe

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.

kinow avatar Nov 06 '21 01:11 kinow

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

garydgregory avatar Nov 06 '21 18:11 garydgregory

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

garydgregory avatar Nov 09 '21 16:11 garydgregory

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 avatar Nov 09 '21 20:11 wodencafe

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

wodencafe avatar Nov 10 '21 05:11 wodencafe

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

garydgregory avatar Nov 12 '21 12:11 garydgregory

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.

garydgregory avatar Jan 30 '22 17:01 garydgregory