Apply @CheckReturnValue to Assert implementations #7038
Conversation
Signed-off-by: Johnny Lim <izeye@naver.com>
shakuzen
left a comment
shakuzen
left a comment
There was a problem hiding this comment.
Looks good. Thanks. Should we add an ArchUnit rule as well to make sure we don't miss this going forward?
This commit also changes Assert implementations to comply with the rules. Signed-off-by: Johnny Lim <izeye@naver.com>
| * @throws AssertionError if there is no matching observation | ||
| */ | ||
| @CheckReturnValue | ||
| public That hasObservationWithNameEqualTo(@Nullable String name) { |
There was a problem hiding this comment.
This seems to read okay without that() and is also being used internally without that() which requires warning suppressions below.
I'm not sure if requiring that() is intentional, but it could be a breaking change for someone using it just for its presence check.
There was a problem hiding this comment.
It does seem a bit awkward compared to other API. Perhaps we can work on migrating to something more consistent. For now, I think it's better to not have @CheckReturnValue here, since it can be used with or without using the return value.
| * @throws AssertionError if the meter is not found | ||
| */ | ||
| @CheckReturnValue | ||
| public MeterAssert<?> meter(String meterName, Tag... tags) { |
There was a problem hiding this comment.
This one and the following ones are similar cases with the above, but they seem to read incomplete.
There was a problem hiding this comment.
I would keep the @CheckReturnValue here. If someone just wants to assert a meter with a name and tags exists, there are other ways to do that and they don't need to use this method.
|
For the build failure, I created #7044 to try to resolve it. |
shakuzen
left a comment
shakuzen
left a comment
There was a problem hiding this comment.
Happy New Year, Johnny! Thanks for updating things. I took a look and left some comments.
| @@ -0,0 +1,59 @@ | |||
| /* | |||
| * Copyright 2025 VMware, Inc. | |||
There was a problem hiding this comment.
Happy New Year! I guess this should be the first 2026 copyright year file
| * @throws AssertionError if there is no matching observation | ||
| */ | ||
| @CheckReturnValue | ||
| public That hasObservationWithNameEqualTo(@Nullable String name) { |
There was a problem hiding this comment.
It does seem a bit awkward compared to other API. Perhaps we can work on migrating to something more consistent. For now, I think it's better to not have @CheckReturnValue here, since it can be used with or without using the return value.
| * @throws AssertionError if the meter is not found | ||
| */ | ||
| @CheckReturnValue | ||
| public MeterAssert<?> meter(String meterName, Tag... tags) { |
There was a problem hiding this comment.
I would keep the @CheckReturnValue here. If someone just wants to assert a meter with a name and tags exists, there are other ways to do that and they don't need to use this method.
This commit also updates ArchUnit tests to cover them. Signed-off-by: Johnny Lim <izeye@naver.com>
shakuzen
added
enhancement
…#7038) * Apply @CheckReturnValue to Assert implementations Signed-off-by: Johnny Lim <izeye@naver.com> * Add ArchUnit tests This commit also changes Assert implementations to comply with the rules. Signed-off-by: Johnny Lim <izeye@naver.com> * Use @CanIgnoreReturnValue for exceptional cases This commit also updates ArchUnit tests to cover them. Signed-off-by: Johnny Lim <izeye@naver.com> --------- Signed-off-by: Johnny Lim <izeye@naver.com>
2 participants
This PR applies
@CheckReturnValueto AssertJAssertimplementations similar to spring-projects/spring-boot#46766.