-
Notifications
You must be signed in to change notification settings - Fork 111
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
Run CI builds for all branches #663
Conversation
3352e63
to
143d8fb
Compare
/** | ||
* SpotBugs looks for an annotation called `SuppressFBWarnings` regardless of the package it is in. | ||
* This will allow us to disable some SpotBugs rules when we want e.g. to allow switch case fallthrough. | ||
*/ |
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.
Is this preferable to having a compile-time dependency on com.github.spotbugs:spotbugs-annotations
?
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.
Yes, this is preferable.
If we introduce spotbugs annotations as a implementation
or runtimeOnly
dependency, either it's a transitive dependency for consumers or it needs to be bundled into the superjar. If it's bundled into the superjar, then we are distributing it and we would need to figure out the implications of distributing software that is licensed as LGPL.
We cannot introduce spotbugs annotations as a compileOnly
dependency, because it will cause there to be an unresolved package when we try to run the tests, or when anyone else tries to use ion-java
without having spotbugs-annotations in their classpath.
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.
Does Spotbugs not run locally? Why did we just start seeing these now?
143d8fb
to
423ddeb
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## ion-11-encoding #663 +/- ##
==================================================
Coverage ? 64.58%
Complexity ? 5681
==================================================
Files ? 199
Lines ? 24603
Branches ? 4382
==================================================
Hits ? 15891
Misses ? 7412
Partials ? 1300 ☔ View full report in Codecov by Sentry. |
Issue #, if available:
None
Description of changes:
I realized that in #661, I forgot to push some changes in one file, and so the code in the PR wouldn't even compile. To prevent that from happening again, I'm making the CI build run for PRs regardless of the branch.
In doing so, I also discovered some SpotBugs warnings. Some of the warnings are for things that we are intentionally doing, so I added a
SuppressFBWarnings
annotation that can be used to mark when we're doing this intentionally. In some cases, I added some code to make the compiler happy.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.