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.

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

Introduce Spotbugs plugin #262

Merged
merged 1 commit into from
Aug 3, 2021
Merged

Introduce Spotbugs plugin #262

merged 1 commit into from
Aug 3, 2021

Conversation

brfrn169
Copy link
Collaborator

This PR introduces the Spotbugs plugin that uses static analysis to look for bugs in Java code. Also, I fixed some Spotbugs errors in this PR.

Honestly, I'm not sure it's a good idea for us to use this plugin because it shows some false positives, and we need to use SuppressFBWarnings annotation to remove them, which might be annoying. But of course, I think it brings many benefits to us. So I would like to discuss whether or not we use this plugin here.

Besides this plugin, I'm thinking that we can use the errorprone plugin, as well:
https://errorprone.info/
https://github.com/tbroyer/gradle-errorprone-plugin

Please take a look!

@brfrn169 brfrn169 self-assigned this Jul 30, 2021
@brfrn169 brfrn169 force-pushed the introduce-spotbugs-plugin branch from 96adc61 to 8748afd Compare July 31, 2021 01:49
Copy link
Contributor

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

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

I think it's a good idea. I agree that it may become troublesome for some of the rules that cause a lot of false positive but I wonder if it will often trigger, personally I think we could try this tool for some time and then see if it's an hindrance or not. Also, I will need to try the Intellij plugin as well.

Besides, thanks for all the fixes 🙂

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

Thank you! The code looks good but I left one question.
It's definitely nice to have so let's see how it goes.
I'll merge after we discuss it tomorrow.

@@ -1,7 +1,5 @@
package com.scalar.db.storage.cassandra;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering what was wrong with the class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was the one to fix a synchronized related warning. Let's discuss it later.

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@feeblefakie feeblefakie requested a review from Torch3333 August 3, 2021 02:37
Copy link
Contributor

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you 🙂

@feeblefakie feeblefakie merged commit 1e3e69a into master Aug 3, 2021
@feeblefakie feeblefakie deleted the introduce-spotbugs-plugin branch August 3, 2021 02:46
brfrn169 added a commit that referenced this pull request Aug 3, 2021
brfrn169 added a commit that referenced this pull request Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants