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

#2674 fixing several resource handling misses #2896

Merged
merged 8 commits into from
Jun 29, 2018

Conversation

svendiedrichsen
Copy link
Contributor

@svendiedrichsen svendiedrichsen commented Jun 24, 2018

Fixes some of the resource handling issues found by SonarQube in #2674

@Cousjava
Copy link
Contributor

Jenkins test please

@payara-ci
Copy link
Contributor

Quick build and test passed!

@smillidge smillidge requested a review from Cousjava June 25, 2018 21:04
@smillidge smillidge added the PR: CLA CLA submitted on PR by the contributor label Jun 25, 2018
@smillidge smillidge added this to the Payara 5.183 milestone Jun 25, 2018
+ " table does not exists. Trying to create it.");
ps = conn.prepareStatement(createTableStatement);
ps.executeUpdate();
try (Statement stmt = conn.createStatement(ResultSet.TYPE_SCROLL_INSENSITIVE,
Copy link
Contributor

Choose a reason for hiding this comment

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

Statement close isn't strictly necessary because when the connection is closed, all of it's statements, and other resources are closed as well.
However, it doesn't hurt and makes for a good coding style as well

@svendiedrichsen
Copy link
Contributor Author

svendiedrichsen commented Jun 26, 2018

@lprimak Sorry, I have found some time to do some more fixes. Please review.

@mulderbaba
Copy link
Contributor

too many code changes in formatting, can you handle them so that we can follow up on what's actually changed.

@mulderbaba
Copy link
Contributor

Jenkins test please

@payara-ci
Copy link
Contributor

Quick build and test failed!

*/
public static PayaraMicroBoot getBootClass() throws InstantiationException, IllegalAccessException, ClassNotFoundException, Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert back this declaration and comment as it helps to make it clear what exceptions may be thrown

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. But I'd argue that this Javadoc does not do much to clarify anything.

@svendiedrichsen
Copy link
Contributor Author

@mulderbaba Do you want me to bring the whitespaces back?

@mulderbaba
Copy link
Contributor

@svendiedrichsen I just want to make sure that I'm not skipping on anything due to heavy load of formatting changes. If you can constrain your changes to the only ones made for resource handling, that'd be great.

@lprimak
Copy link
Contributor

lprimak commented Jun 27, 2018

GitHub diff screen has a setting "hide whitespace changes" that was very helpful in this case

@lprimak
Copy link
Contributor

lprimak commented Jun 27, 2018

jenkins test

@payara-ci
Copy link
Contributor

Quick build and test passed!

@svendiedrichsen
Copy link
Contributor Author

@mulderbaba As @lprimak noted you can make Github show the diff without spaces.

@lprimak lprimak merged commit eaeea99 into payara:master Jun 29, 2018
@svendiedrichsen svendiedrichsen deleted the 2674_RESOURCE_HANDLING branch June 29, 2018 05:41
Cousjava added a commit to Cousjava/Payara that referenced this pull request Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: CLA CLA submitted on PR by the contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants