-
Notifications
You must be signed in to change notification settings - Fork 670
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
[IO-814] Don't throw UncheckedIOException #491
Conversation
There was a problem hiding this comment.
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.
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 Report
@@ 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
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Reverted Javadoc nits. PTAL |
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? |
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.