-
Notifications
You must be signed in to change notification settings - Fork 289
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
Added converted to result method on FxConvertible #2643
base: main
Are you sure you want to change the base?
Conversation
@@ -239,6 +245,19 @@ public void test_convertedTo_rateProvider() { | |||
assertThat(CCY_AMOUNT.convertedTo(CCY1, provider)).isEqualTo(CCY_AMOUNT); | |||
} | |||
|
|||
@Test | |||
public void test_convertedToResult_failure() { |
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.
Can you add a test for the success case
* @return the result of the converted instance, which should be expressed in the specified currency | ||
*/ | ||
public default Result<R> convertedToResult(Currency resultCurrency, FxRateProvider rateProvider) { | ||
return Result.of(() -> convertedTo(resultCurrency, rateProvider)); |
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.
Given this implementation, I'm struggling to justify adding the method,. Bear in mind that the method becomes public on a large number of implementation classes.
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.
We have Result.of(() -> ...convertedTo)
littered around most of the models, as we want to avoid any hard errors in a calculation, this method would help clean things up a lot.
No description provided.