-
-
Notifications
You must be signed in to change notification settings - Fork 690
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
Add hasPrecisionOf(int)
assertion for BigDecimal
#3174
Conversation
hasPrecisionOf(int)
assertion for BigDecimalhasPrecisionOf(int)
assertion for BigDecimal
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 @armandino! My comments are mostly due to the hasScale()
/ scale()
patterns you probably followed. We would prefer to follow a different style for new assertions.
We might also improve hasScale()
/ scale()
but this can be taken up in a separate issue or PR.
assertj-core/src/main/java/org/assertj/core/api/AbstractBigDecimalAssert.java
Outdated
Show resolved
Hide resolved
assertj-core/src/main/java/org/assertj/core/internal/BigDecimals.java
Outdated
Show resolved
Hide resolved
assertj-core/src/main/java/org/assertj/core/error/ShouldHavePrecision.java
Show resolved
Hide resolved
...c/test/java/org/assertj/core/api/bigdecimal/BigDecimalsAssert_assertHasPrecisionOf_Test.java
Outdated
Show resolved
Hide resolved
*/ | ||
package org.assertj.core.api; | ||
|
||
public class BigDecimalPrecisionAssert<T> extends AbstractBigDecimalPrecisionAssert<BigDecimalAssert> { |
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.
T
seems to be unused, isn't it?
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 tried removing it, but looks like it is required here:
public AbstractBigDecimalPrecisionAssert<SELF> precision() {
isNotNull();
return new BigDecimalPrecisionAssert(myself);
}
...ore/src/test/java/org/assertj/core/internal/bigdecimals/BigDecimalAssert_precision_Test.java
Outdated
Show resolved
Hide resolved
@SuppressWarnings({ "unchecked", "rawtypes" }) | ||
public AbstractBigDecimalPrecisionAssert<SELF> precision() { | ||
requireNonNull(actual, "Can not perform assertions on the precision of a null BigDecimal"); | ||
return new BigDecimalPrecisionAssert(myself); |
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 briefly discussed this approach with the team.
We should add a bit of context to the error messages by showing the root object we navigated from.
For example, assuming:
assertThat(myBigDecimal).scale().isGreaterThan(5L)
If the assertion fails, it will show something like
expecting 2 to be greater than 5 but was not
where the fact that a BigDecimal
precision was checked is forgotten.
Instead, the error message should have a better description and be more like:
[checking 2.123 precision] expecting 2 to be greater than 5 but was not
Unfortunately, we don't do this consistently in AssertJ so I didn't manage to find a good example quickly. I'll try to have another look in the next few days.
@scordio thanks for the feedback! It was very helpful. I addressed most of the comments, but there are still a few outstanding items. |
Hi @scordio, I updated the PR. Please let me know if I missed anything. |
Check List:
AssertJ provides a
hasScaleOf()
assertion for theBigDecimal
type. This PR adds a companion assertionhasPrecisionOf
for verifying theBigDecimal.precision()
method. I followed thehasScaleOf()
implementation and tests in this PR.