Skip to content

[IO-814] Don't throw UncheckedIOException#491

Merged
garydgregory merged 7 commits into
apache:masterfrom
elharo:IO-814
Oct 7, 2023
Merged

[IO-814] Don't throw UncheckedIOException#491
garydgregory merged 7 commits into
apache:masterfrom
elharo:IO-814

Conversation

@elharo

@elharo elharo commented Oct 6, 2023

Copy link
Copy Markdown
Contributor

Fortunately the API here offered a backwards compatible fix. Since it was already declared to return null for some exceptions, this makes it return null for IOException too. This reverts the behavior breaking change without adding any API changes to anyone's code.

Also some javadoc cleanups while I was in here.

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please don't change the style: Sentences start with a capital letter and end with a period.

@elharo

elharo commented Oct 6, 2023

Copy link
Copy Markdown
Contributor Author

Except they aren't sentences. They're sentence fragments, and this case is explicitly called out in the Oracle style guidelines: "When writing the comments themselves, in general, start with a phrase and follow it with sentences if they are needed. When writing a phrase, do not capitalize and do not end with a period".

If the project wants to diverge from that, it's worth calling it out explicitly in a project specific style guide.

@garydgregory

Copy link
Copy Markdown
Member

Except they aren't sentences. They're sentence fragments, and this case is explicitly called out in the Oracle style guidelines: "When writing the comments themselves, in general, start with a phrase and follow it with sentences if they are needed. When writing a phrase, do not capitalize and do not end with a period".

If the project wants to diverge from that, it's worth calling it out explicitly in a project specific style guide.

We do, under a header called "Respect The Original Style" on https://commons.apache.org/patches.html

I suppose it could go into more detail of course ;-)

@codecov-commenter

codecov-commenter commented Oct 7, 2023

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.03%. Comparing base (877b9e3) to head (e21541f).
⚠️ Report is 1750 commits behind head on master.

Files with missing lines Patch % Lines
...ain/java/org/apache/commons/io/file/PathUtils.java 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #491      +/-   ##
============================================
+ Coverage     84.97%   85.03%   +0.06%     
- Complexity     3372     3375       +3     
============================================
  Files           227      227              
  Lines          8080     8080              
  Branches        953      953              
============================================
+ Hits           6866     6871       +5     
+ Misses          960      957       -3     
+ Partials        254      252       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@elharo

elharo commented Oct 7, 2023

Copy link
Copy Markdown
Contributor Author

Reverted Javadoc nits. PTAL

@garydgregory garydgregory merged commit 606e72f into apache:master Oct 7, 2023
asfgit pushed a commit that referenced this pull request Oct 7, 2023
@elharo elharo deleted the IO-814 branch October 7, 2023 16:31
@jglick

jglick commented Jan 29, 2024

Copy link
Copy Markdown
Contributor

Would a variant of the test from #478 be useful here as a regression test? Like

PathUtils.deleteFile(tempDir.resolve("nonexistent").resolve("file-does-not-exist.bin"), StandardDeleteOption.OVERRIDE_READ_ONLY));

which if I understand correctly should now return normally?

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