-
Notifications
You must be signed in to change notification settings - Fork 41
Introduce AssertThatHasValue
Refaster rule
#1547
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: master
Are you sure you want to change the base?
Introduce AssertThatHasValue
Refaster rule
#1547
Conversation
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.
Ahh nice that you opened the PR 🚀 !
We can probably add one case to the Refaster rule above here AbstractOptionalAssertHasValue
.
The build will probably fail as we also need to add tests 😄. Try running mvn clean install
to see whether the build passes 😉.
Maybe we can go over it together? Because IIUC we cannot add the following: If we stay with the new function, should it maybe also return
On it! |
Looks good. No mutations were possible for these changes. |
|
optional.orElseThrow()
assertionAssertThatHasValue
Refaster rule
bdd777a
to
f49d20d
Compare
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.
Congrats on your first contribution, @JulianBroudy! 🎉 Too bad this is a tricky one. 😬
I rebased and added a commit to show the issue. Normally for this kind of change we would use this commit message:
Introduce `AssertThatHasValue` Refaster rule (#1547)
But as this approach won't work, we'll have to consider an alternative: introducing an AssertThatHasValue
Error Prone checker, just like we did for AssertJIsNull
. This variant would be slightly more complex, but not by much. Are you up for that? I'm sure @rickie is up for providing some pointers :)
@@ -103,6 +104,19 @@ AbstractOptionalAssert<?, T> after(AbstractOptionalAssert<?, T> optionalAssert, | |||
} | |||
} | |||
|
|||
static final class AssertThatHasValue<T> { | |||
@BeforeTemplate | |||
AbstractStringAssert<?> before(Optional<String> optional, String value) { |
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.
As-is, this rule will only match Optional<String>
assertions, while we would like to match any Optional<T>
assertion:
AbstractStringAssert<?> before(Optional<String> optional, String value) { | |
ObjectAssert<T> before(Optional<T> optional, T value) { |
... but then we find that the tests fails, because of how Refaster works: the rule will only match the used assertThat
overload, while there are many.
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 @Stephan202, always up for anything!
We started with having it as generic and then went on to making it specific for each of String, Boolean, etc. Finally decided to stay only with String for now. I will take another look with @rickie.
Refaster:
to:
Suggested Commit Message 🎉