Skip to content

Fix some minor findbugs warnings#67

Closed
mdkf wants to merge 5 commits intojenkinsci:masterfrom
mdkf:master
Closed

Fix some minor findbugs warnings#67
mdkf wants to merge 5 commits intojenkinsci:masterfrom
mdkf:master

Conversation

@mdkf
Copy link
Copy Markdown

@mdkf mdkf commented Feb 21, 2017

No description provided.

Michael Fowler added 3 commits February 21, 2017 00:12
@pjdarton
Copy link
Copy Markdown
Member

As mentioned in #76, I now have permission to process pull requests on this plugin. As a result, I've been reviewing lots of outstanding pull requests.

I've not merged #67 because I don't see these FindBugs warnings in either my IDE (I use Eclipse) or in the Jenkins build (findbugs doesn't seem to be being called) and I'm wary of making changes where I can't see the impact.
I would like us to have findbugs run over the code if we can, as code-analysis checking tools like this can find silly errors much more quickly than manual testing does, but it isn't something I know much about (I've never dealt with findbugs or telling maven to call it).
Also, the other changes made mean that this PR now has conflicts, so it's no longer a single button-push.

If you can walk me through how to verify these changes - where findbugs reports should show up, where the build should fail if there are issues found etc - and how to ensure that they're run every build, and update the PR so it can be merged, then I'll give it a shot.

pjdarton added a commit to pjdarton/vsphere-cloud-plugin that referenced this pull request Sep 5, 2017
jenkinsci#67 fixes a number
of findbugs warnings but then failed to build and failed to merge.
This change includes a number of the "safer" changes from that pull
request:
 - Remove unnecessary imports.
 - Add missing @OverRide annotations.
 - Add "final" keyword when appropriate.
 - Use jenkins.model.Jenkins instead of Hudson
 - Update org.jenkins-ci.plugins/plugin version from 2.6 to 2.22
pjdarton added a commit that referenced this pull request Sep 5, 2017
pjdarton added a commit to pjdarton/vsphere-cloud-plugin that referenced this pull request Sep 6, 2017
Add the line <?jelly escape-by-default='true'?> where it's absent.
Plus some whitespace changes where we had previously a mix of tabs and
spaces.
pjdarton added a commit to pjdarton/vsphere-cloud-plugin that referenced this pull request Sep 6, 2017
Removed unnecessary cast.
Removed unnecessary import.
pjdarton added a commit to pjdarton/vsphere-cloud-plugin that referenced this pull request Sep 6, 2017
Refactored excessively long if/elseif...else clauses as a switch.
pjdarton added a commit to pjdarton/vsphere-cloud-plugin that referenced this pull request Sep 6, 2017
Change version of org.jenkins-ci.plugins/plugin from 2.22 to 2.29, which
is the last version that works with a Java7 JDK.
PR jenkinsci#67 upgraded to 2.30 which is Java8-only, and I'm hoping to avoid
that at present.
@pjdarton pjdarton mentioned this pull request Sep 6, 2017
pjdarton added a commit that referenced this pull request Sep 6, 2017
@pjdarton
Copy link
Copy Markdown
Member

pjdarton commented Sep 7, 2017

I've merged these changes, bit by bit, in #87 and #88.
The only bit I've not taken was updating the org.jenkins-ci.plugins/plugin version to 2.30 (as that forces us onto Java8 and I don't want that yet).

@pjdarton pjdarton closed this Sep 7, 2017
@mdkf
Copy link
Copy Markdown
Author

mdkf commented Sep 12, 2017

Thanks @pjdarton !

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants