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

CheckReturnValue for assertThat statements for Findbugs #695

Merged
merged 1 commit into from Jul 1, 2016

Conversation

Projects
None yet
2 participants
@karlicoss
Contributor

karlicoss commented Jun 27, 2016

Check List:

  • Fixes #157
  • Unit tests : NA
  • Javadoc with a code example (API only) : NA

I've found this, dug into Findbugs code, and apparently, Findbugs handles any annotation with name CheckReturnValue in its return value usage check. So, if we supply our own annotation, we can avoid being dependent on jsr305 package.

If you approve it, I'll search for other classes in assertJ where it's reasonable to put this annotation and also will backport to 2.x version as well.

I also checked it against my build of this branch, it totally works! See https://github.com/karlicoss/assertj-return-value-demo

@joel-costigliola

This comment has been minimized.

Show comment
Hide comment
@joel-costigliola

joel-costigliola Jun 27, 2016

Owner

That is awesome !

Do you think it is possible to customise the report ? I was looking at your project sample report but it does not explain what was wrong with the code, reporting a problem with an unrelevant explanation might confuse people.

Owner

joel-costigliola commented Jun 27, 2016

That is awesome !

Do you think it is possible to customise the report ? I was looking at your project sample report but it does not explain what was wrong with the code, reporting a problem with an unrelevant explanation might confuse people.

@karlicoss

This comment has been minimized.

Show comment
Hide comment
@karlicoss

karlicoss Jun 27, 2016

Contributor

@joel-costigliola yeah, I thought about that too (although, it's a waaay smaller problem than useless assertions :) ). It might be possible to customize it, I'll take a look tomorrow.

Contributor

karlicoss commented Jun 27, 2016

@joel-costigliola yeah, I thought about that too (although, it's a waaay smaller problem than useless assertions :) ). It might be possible to customize it, I'll take a look tomorrow.

@joel-costigliola

This comment has been minimized.

Show comment
Hide comment
@joel-costigliola
Owner

joel-costigliola commented Jun 27, 2016

👍

@karlicoss

This comment has been minimized.

Show comment
Hide comment
@karlicoss

karlicoss Jun 28, 2016

Contributor

@joel-costigliola Looks like there is no way to customize error message :( Anyway, I feel like people who use findbugs are smart enough to figure that out or google :)

Contributor

karlicoss commented Jun 28, 2016

@joel-costigliola Looks like there is no way to customize error message :( Anyway, I feel like people who use findbugs are smart enough to figure that out or google :)

@joel-costigliola

This comment has been minimized.

Show comment
Hide comment
@joel-costigliola

joel-costigliola Jun 28, 2016

Owner

Thanks for the investigation.

Can you also annotate : BDDAssertions and AbstractStandardSoftAssertions

Owner

joel-costigliola commented Jun 28, 2016

Thanks for the investigation.

Can you also annotate : BDDAssertions and AbstractStandardSoftAssertions

@karlicoss

This comment has been minimized.

Show comment
Hide comment
@karlicoss

karlicoss Jun 29, 2016

Contributor

@joel-costigliola updated the PR.
I actually annotated every method returning assertion rather than the whole classes because not all of these methods return assertions and should be chain-called. For instance, Assertions.contentOf or Assertions.withinPercentage. Hope I didn't miss anything.

In case someone is curious, I used a script to add the annotations:

find src/main/java/org/assertj/core/api -type f -iname "*Assertions*.java" -exec sed -i 's/^[ tab]\+public .*Assert.* [a-zA-Z0-9]\+(.*).*$/  @CheckReturnValue\n&/' {} \;
Contributor

karlicoss commented Jun 29, 2016

@joel-costigliola updated the PR.
I actually annotated every method returning assertion rather than the whole classes because not all of these methods return assertions and should be chain-called. For instance, Assertions.contentOf or Assertions.withinPercentage. Hope I didn't miss anything.

In case someone is curious, I used a script to add the annotations:

find src/main/java/org/assertj/core/api -type f -iname "*Assertions*.java" -exec sed -i 's/^[ tab]\+public .*Assert.* [a-zA-Z0-9]\+(.*).*$/  @CheckReturnValue\n&/' {} \;

@joel-costigliola joel-costigliola merged commit 9c9e731 into joel-costigliola:master Jul 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@joel-costigliola

This comment has been minimized.

Show comment
Hide comment
@joel-costigliola

joel-costigliola Jul 1, 2016

Owner

@karlicoss thanks !

If you can do the backport PR for this week end as I'm planning to relase 2.5.0/3.5.0 this sunday.
If you can't, that's ok, I'll do it

Owner

joel-costigliola commented Jul 1, 2016

@karlicoss thanks !

If you can do the backport PR for this week end as I'm planning to relase 2.5.0/3.5.0 this sunday.
If you can't, that's ok, I'll do it

@karlicoss

This comment has been minimized.

Show comment
Hide comment
@karlicoss

karlicoss Jul 1, 2016

Contributor

Yeah, I'll open the PR today once I get to work!

Dima Gerasimov, ITMO University, study group A4200
On Jul 1, 2016 11:16 AM, "Joel Costigliola" notifications@github.com
wrote:

@karlicoss https://github.com/karlicoss thanks !

If you can do the backport PR for this week end as I'm planning to relase
2.5.0/3.5.0 this sunday.
If you can't, that's ok, I'll do it


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#695 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AARyBaF6-QN3AmxtsXccNAhI34DgKHhUks5qRMzogaJpZM4I_Bf3
.

Contributor

karlicoss commented Jul 1, 2016

Yeah, I'll open the PR today once I get to work!

Dima Gerasimov, ITMO University, study group A4200
On Jul 1, 2016 11:16 AM, "Joel Costigliola" notifications@github.com
wrote:

@karlicoss https://github.com/karlicoss thanks !

If you can do the backport PR for this week end as I'm planning to relase
2.5.0/3.5.0 this sunday.
If you can't, that's ok, I'll do it


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#695 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AARyBaF6-QN3AmxtsXccNAhI34DgKHhUks5qRMzogaJpZM4I_Bf3
.

@joel-costigliola

This comment has been minimized.

Show comment
Hide comment
@joel-costigliola

joel-costigliola Jul 1, 2016

Owner

sweet :)

Owner

joel-costigliola commented Jul 1, 2016

sweet :)

karlicoss pushed a commit to karlicoss/assertj-core that referenced this pull request Jul 1, 2016

@karlicoss

This comment has been minimized.

Show comment
Hide comment
@karlicoss
Contributor

karlicoss commented Jul 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment