API - Deprecate unused runSafely functions #5205
Conversation
|
I'd be ok with removing these now, though they are part of API so we might want to deprecate for at least one release. |
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| public class TestExceptionUtil { |
There was a problem hiding this comment.
Why remove the test if the tested code is still in the codebase? Even if deprecated, it'd be nice to validate that it works. These tests do not look too expensive to run 🤔
There was a problem hiding this comment.
Happy to add them back!
There was a problem hiding this comment.
nit: update the PR's summary line?
There was a problem hiding this comment.
Updated. I'm going to add a note about removing in 1.0 as well.
kbendick
changed the title
…emoved and mentioning the unit test so we don't forget to remove those tooo
|
cc @RussellSpitzer I'm not sure if you guys use these functions internally. They've always given somewhat difficult to reason about error prone warnings, and I discovered they aren't used at all (outside of in their associated tests). So I deprecated them but wanted to give you a heads up. |
|
We do not use these, +1 |
|
Ryan mentioned that since this code is tested and works, we might want to have it still. So I'm opening a PR to fix a small bug in the thrown combined |
4 participants
When cleaning up some error prone warnings, I noticed that the
ExceptionUtil.runSafelymethods are not used anywhere other than from their own tests.Given that we are going to have API stability guarantees come 1.0, we should consider deprecating these for 0.14.0 and then removing them at 1.0 so we're not giving API / ABI compatibility guarantees for code that isn't used within the library and gives error prone warnings that are difficult to reason about.
Original PR where the discussion of these functions took place: #5190