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

Generify throwable in CheckedCallable/CheckedRunnable #3326

Merged
merged 3 commits into from
Aug 3, 2022

Conversation

shakuzen
Copy link
Member

@shakuzen shakuzen commented Aug 2, 2022

This allows usage that catches or throws more a more specific type than Throwable. This is a bit more verbose when declaring a variable of type CheckedCallable or CheckedRunnable, but I expect most usage to be as a lambda or method reference, which avoids the need to do such.

@jonatan-ivanov let me know what you think. I may have overlooked some reason why not to do this. This change wasn't based on user feedback, so I don't feel strongly about doing this but I figured I would open the proposal for discussion.

This allows usage that catches or throws more a more specific type than `Throwable`. This is a bit more verbose when declaring a variable of type CheckedCallable or CheckedRunnable, but I expect most usage to be as a lambda or method reference, which avoids the need to do such.
@shakuzen shakuzen added this to the 1.10.0-M4 milestone Aug 2, 2022
String s = observation.scopedChecked(service::executeCallable);
observation.scopedChecked(service::executeRunnable);
}
catch (IOException ignore) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This wasn't possible before due to it being hard-coded to Throwable

Make it explicit whether CheckedRunnable or CheckedCallable is being tested and avoid the wrong overload of the method being called in the test.
@shakuzen
Copy link
Member Author

shakuzen commented Aug 3, 2022

This is a breaking change if someone is declaring a CheckedCallable variable somewhere like the following because CheckedCallable now requires two generic arguments.

Observation.CheckedCallable<String> callable = () -> {
            assertThat(registry.getCurrentObservation()).isNull();
            return "test";
        };

CheckedRunnable will still compile because it had 0 before.
I am hoping/expecting that the impact of this breaking change will be minimal since I expect explicit declarations of CheckedCallable to be rare (if exist at all).

@shakuzen shakuzen merged commit 539c4fe into micrometer-metrics:main Aug 3, 2022
@shakuzen shakuzen deleted the generify-throws-type branch August 3, 2022 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants