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.

Sign up for GitHub

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

Jump to bottom

Drop rules from AssertJBigDecimalTemplates #30

Merged
merged 3 commits into from
Apr 11, 2022
Merged

Drop rules from AssertJBigDecimalTemplates #30

merged 3 commits into from
Apr 11, 2022

Conversation

rickie
Copy link
Member

@rickie rickie commented Jan 6, 2022 •
edited
Loading

We drop these rules because they result in non-compilable code.

@rickie rickie requested a review from Stephan202 January 6, 2022 19:45
@rickie
Copy link
Member Author

rickie commented Mar 5, 2022

Suggested commit message:

`AssertJBigDecimalTemplates`: drop assorted incorrect candidates

Due to the scale of `BigDecimal` we would reach a non-compilable state when 
performing a rewrite.

Badbond
Badbond reviewed Mar 7, 2022
View reviewed changes
...ntrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJBigDecimalTemplates.java Outdated
Comment on lines 21 to 24
* <li>{@link BigDecimalAssert#isEqualTo(Object)} (for values 0L, 1L, BigDecimal.ZERO, and
* BigDecimal.ONE)
* <li>{@link BigDecimalAssert#isNotEqualTo(Object)} (for values 0L, 1L, BigDecimal.ZERO, and
* BigDecimal.ONE)
Copy link
Member

@Badbond Badbond Mar 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The implementation appears to indicate that we can rewrite is[Not]EqualTo for values BigDecimal.ZERO and BigDecimal.ONE to isZero() and isOne() resp., correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good one, tried to make it more clear, but not really happy with the result. WDYT?

Badbond
Badbond approved these changes Mar 8, 2022
View reviewed changes
Copy link
Member

@Badbond Badbond left a comment

Choose a reason for hiding this comment

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

Looking good! 🚀

@rickie rickie added this to the 0.1.0 milestone Apr 4, 2022
Stephan202
Stephan202 reviewed Apr 6, 2022
View reviewed changes
...ntrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJBigDecimalTemplates.java Outdated
}

@AfterTemplate
AbstractBigDecimalAssert<?> after(AbstractBigDecimalAssert<?> bigDecimalAssert) {
return bigDecimalAssert.isEqualTo(0);
return bigDecimalAssert.isZero();
Copy link
Member

Choose a reason for hiding this comment

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

If we now rewrite to isZero() instead if isEqualTo(0), then we can keep the bigDecimalAssert.isEqualTo(0L) rewrite, I would expect, since isZero() uses comparison.

But... since we don't advocate isZero() for other types of NumberAssert, I wonder whether we should perform this rewrite at all. Maybe something to quickly sync on offline.

@Stephan202 Stephan202 force-pushed the rossendrijver/drop_bigdecimal_templates branch from 8edfe77 to 3480d93 Compare April 10, 2022 09:31
Stephan202
Stephan202 approved these changes Apr 10, 2022
View reviewed changes
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Rebased and added a commit with a slightly different approach, in line with what we discussed offline. PTAL.

Suggested commit message:

Drop or rewrite `AssertJBigDecimalTemplates` rules (#30)

@rickie
Copy link
Member Author

rickie commented Apr 11, 2022

Changes LGTM, nice concise explanation of the discussion 🚀 !

@Stephan202 Stephan202 merged commit 2199acc into master Apr 11, 2022
@Stephan202 Stephan202 deleted the rossendrijver/drop_bigdecimal_templates branch April 11, 2022 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Badbond Badbond Badbond approved these changes

@Stephan202 Stephan202 Stephan202 approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
0.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@rickie @Stephan202 @Badbond