Skip to content
This repository was archived by the owner on Jul 25, 2020. It is now read-only.

Address FindBugs issues and disable some detectors#498

Closed
gaul wants to merge 2 commits intojclouds:masterfrom
gaul:findbugs
Closed

Address FindBugs issues and disable some detectors#498
gaul wants to merge 2 commits intojclouds:masterfrom
gaul:findbugs

Conversation

@gaul
Copy link
Copy Markdown
Member

@gaul gaul commented Aug 27, 2014

This commit fixes enough FindBugs issues and disables enough detectors for mvn findbugs:check to pass.

@cloudbees-pull-request-builder
Copy link
Copy Markdown

jclouds-pull-requests-java-6 #30 SUCCESS
This pull request looks good

@buildhive
Copy link
Copy Markdown

jclouds » jclouds #1558 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

@cloudbees-pull-request-builder
Copy link
Copy Markdown

jclouds-pull-requests #1119 UNSTABLE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder
Copy link
Copy Markdown

jclouds-pull-requests-java-6 #31 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder
Copy link
Copy Markdown

jclouds-pull-requests-java-6 #32 SUCCESS
This pull request looks good

@buildhive
Copy link
Copy Markdown

jclouds » jclouds #1559 SUCCESS
This pull request looks good
(what's this?)

@cloudbees-pull-request-builder
Copy link
Copy Markdown

jclouds-pull-requests #1120 UNSTABLE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder
Copy link
Copy Markdown

jclouds-pull-requests #1121 SUCCESS
This pull request looks good

@buildhive
Copy link
Copy Markdown

jclouds » jclouds #1560 SUCCESS
This pull request looks good
(what's this?)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm guessing the elements listed are also those included in the toString?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correct. Note that some tests actually fail when I included credential and credentialUrl.

@demobox
Copy link
Copy Markdown
Member

demobox commented Aug 28, 2014

Thanks, @andrewgaul! One thing I noticed was that there are a couple of cases (I commented on two but didn't check further) where we are replacing X.valueOf with X.parseX even though the required value is actually the wrapper object (as returned by valueOf), not the primitive as returned by parseX.

Is there any benefit to doing so? Otherwise, it seems to me that using the parsing method that actually returns the required type, and not relying on autoboxing, would be more logical?

@gaul
Copy link
Copy Markdown
Member Author

gaul commented Aug 28, 2014

This pull request has two commented out sections that I do not know how to address and appreciate suggestions. If we cannot find a solution I will remove the changes and disable those detectors as well.

In terms of parseInt vs. valueOf, there former is always at least as efficient when the caller expects an Integer and slightly more efficient when the caller expects an int. We could add a Checkstyle regexp to prevent use of valueOf entirely.

@gaul
Copy link
Copy Markdown
Member Author

gaul commented Aug 30, 2014

I pushed the agreed-upon changes to master as 63d43f2, 5399cb6, 195998b, and fdef97d. I updated the pull request with the amended changes.

@cloudbees-pull-request-builder
Copy link
Copy Markdown

jclouds-pull-requests-java-6 #40 SUCCESS
This pull request looks good

@buildhive
Copy link
Copy Markdown

jclouds » jclouds #1573 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

@cloudbees-pull-request-builder
Copy link
Copy Markdown

jclouds-pull-requests #1129 ABORTED

@cloudbees-pull-request-builder
Copy link
Copy Markdown

jclouds-pull-requests-java-6 #47 SUCCESS
This pull request looks good

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants