Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IOUtils chain together IOException for Closeables, add overload #300

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

wodencafe
Copy link
Contributor

This pull request chains together IOExceptions thrown from closing multiple Closeable instances, which allows the method to attempt to close all of the Closeable instances rather than halting at the first IOException. An overload is added to allow closing multiple Closeable instances and passing the possible resulting IOException to an IOConsumer.

@garydgregory
Copy link
Member

-1: This makes no sense to me as the exception are not causaly unrelated and you are forcing them to be. This might be a use case for IOExceptionList.

@wodencafe
Copy link
Contributor Author

Hi @garydgregory , good suggestion, IOExceptionList seems like the perfect fit for this. I have revised the code to use this class.

@garydgregory
Copy link
Member

I ended up reworking the underlying so that now the method looks like this

    public static void close(final Closeable... closeables) throws IOException {
        IOConsumer.forEach(closeables, IOUtils::close);
    }

See also forEachIndex which provides slightly different information for compatibility with other call sites.

@garydgregory
Copy link
Member

Hi @wodencafe Thank you for your update. Now I see one new method, without any tests, and not used from anywhere.

  • Should the [] be changed to a ... and moved to the last parameter position? I would think so.
  • Can the method be used within the code base?
  • There are no tests.

@wodencafe
Copy link
Contributor Author

Hello @garydgregory , my apologies, I got swamped this weekend and haven't updated this PR with tests. I also agree that the [] should be changed to ... varargs and swapped to be the last parameter, so I will implement this change, and see if there are other places in Commons IO that can take advantage of this new functionality. I will add tests for this today.

Thank you for reviewing the changes, and the contributions and feedback.

@wodencafe wodencafe force-pushed the IOUtils-closewitherrors branch 4 times, most recently from 03f7fcd to 51759a5 Compare November 11, 2021 15:42
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 87.329% when pulling 1ccb075 on wodencafe:IOUtils-closewitherrors into 5300267 on apache:master.

@wodencafe
Copy link
Contributor Author

Hi @garydgregory , I have updated the pull request with the changes you suggested.
Thank you!

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.

4 participants