-
Notifications
You must be signed in to change notification settings - Fork 134
refactor: make the onFailure to be typed when we use onFailure(RuntimeException.class) #1848
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
base: main
Are you sure you want to change the base?
Conversation
802f2f1
to
a430044
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1848 +/- ##
============================================
+ Coverage 88.89% 88.90% +0.01%
+ Complexity 3062 3058 -4
============================================
Files 409 409
Lines 13029 13033 +4
Branches 1642 1642
============================================
+ Hits 11582 11587 +5
+ Misses 821 817 -4
- Partials 626 629 +3
🚀 New features to boost your workflow:
|
a430044
to
80110a1
Compare
Not a review: I would use |
80110a1
to
aefa34a
Compare
good point. updated. |
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.
Thanks a lot for looking into this, this is something we could have done better in the early days of Mutiny since onFailure
with a guard should indeed have a more precise type in the next action.
That being said this introduces binary breaking changes, so I'd like to wait a bit before we decide to go ahead with this PR. This might land in a major release in a few months instead.
I will also ping colleagues for reviews / opinions.
*/ | ||
public class UniOnFailure<T> { | ||
public class UniOnFailure<T, TT extends Throwable> { |
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.
Overall I would rename the TT
parametric type to E
(for error).
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.
You should add more unit tests to show the failure type specialization in the API, especially with all the impacted groups / operators.
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.
I addressed your comments and also I have added few tests to cover the touched methods.
let me know if you think I missed anything.
@ozangunalp @cescoffier I've asked for review, but note that there is no rush for this PR. We should eventually consider it for Mutiny 3 I think due to the presence of binary breaking changes (source-level should be fine, but I've asked @hadadil to increase test coverage here so we see more of the new usage) |
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.
I like the idea, and yes, we should have done that since day 1 (my fault).
f68c6e0
to
50656fb
Compare
…eException.class) Signed-off-by: Laszlo Hadadi <[email protected]>
50656fb
to
695b604
Compare
Do we have any road map for mutiny ex: 3.x? similar to quarkus wanna see this breaking change soon. |
Signed-off-by: Laszlo Hadadi <[email protected]>
Signed-off-by: Laszlo Hadadi <[email protected]>
695b604
to
7ac49be
Compare
We don't have roadmap yet, but we'll sort it by mid-June I think. Meanwhile you can always apply the patch on your own build of Mutiny (not ideal I know, but...) |
When I am using mutiny many times I would like to handle a given type of Exception nicelly to recover or convert the exception.
in this case the
someException
is type ofSomeException
so we can operate on this exception type safe and not as Throwable anymore.in the past I have to do something like this