Skip to content

[IO-808] Rationalize and unify checking for existence of files and directories#529

Merged
garydgregory merged 2 commits into
apache:masterfrom
elharo:IO-808-B
Dec 21, 2023
Merged

[IO-808] Rationalize and unify checking for existence of files and directories#529
garydgregory merged 2 commits into
apache:masterfrom
elharo:IO-808-B

Conversation

@elharo

@elharo elharo commented Dec 20, 2023

Copy link
Copy Markdown
Contributor

The existing code is a bit of a hodge-podge. This PR moves most current IllegalArgumentExceptions for non-existent files and directories to FileNotFoundExceptions instead. A few turned into UncheckedIOExceptions to maintain API compatibility. The general principle is:

  • If a file or directory is not found, throw a FileNotFoundException (or UncheckedIOException if necessary for backwards compatibility)
  • if a file is found where a directory is expected or vice versa, throw an IllegalArgumentException

IllegalArgumentExceptions are still thrown in the case where a file object exists but is a file instead of the expected directory or a directory instead of the expected file. I've come to understand that this is wrong too, but this is existing behavior that can be improved in a subsequent PR.

In addition to correctly reporting missing files, this PR removes a number of duplicate checks and methods, and tries to clarify the names of private methods, e.g. by distinguishing whether we're checking for a file, directory, or file object.

Some further changes that should happen here would break the API and require a major version update.

@garydgregory

@garydgregory

Copy link
Copy Markdown
Member

@elharo
Please run 'mvn' locally before you push to avoid all these failures.

@garydgregory garydgregory merged commit 33db634 into apache:master Dec 21, 2023
asfgit pushed a commit that referenced this pull request Dec 21, 2023
@garydgregory

Copy link
Copy Markdown
Member

Hi @elharo

PR merged, TY. I think we might now hear complaints that not all IO exceptions are documented. For example: FileUtils.checksum(File, Checksum) does document IOException but not FileNotFoundException. Shouldn't all (checked) exceptions be documented? I might have mistakenly assumed this was a goal for this PR.

@elharo elharo deleted the IO-808-B branch December 21, 2023 12:59
@elharo

elharo commented Dec 21, 2023

Copy link
Copy Markdown
Contributor Author

It doesn't hurt to add more detailed exception documentation. If there are any public methods where FileNotFoundException is thrown and neither FileNotFoundException nor IOException is documented, then that should be fixed. FileNotFoundException is a subclass of IOException so it's not absolutely necessary to document it separately though it can be.

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.

2 participants