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

Encourage try-with-resource #236

Merged
merged 1 commit into from
Jan 31, 2023
Merged

Encourage try-with-resource #236

merged 1 commit into from
Jan 31, 2023

Conversation

exoego
Copy link
Contributor

@exoego exoego commented Jan 29, 2023

try-with-resource is highly encouraged since it ensure resource closing with simpler code

@exoego exoego added the doc label Jan 29, 2023
@exoego exoego marked this pull request as ready for review January 29, 2023 06:12
@yuokada yuokada added chore internal changes (not user-facing) and removed doc labels Jan 30, 2023
Copy link
Contributor

@yuokada yuokada left a comment

Choose a reason for hiding this comment

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

Basically, LGTM. One comment.

}
catch (Exception e) {
logger.error("Failed to delete query: {}", name, e);
try (TDClient client1 = client;
Copy link
Contributor

@yuokada yuokada Jan 31, 2023

Choose a reason for hiding this comment

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

I'm not sure whether it's necessary to use try-with-resource statement in this case.

The resources are declared in another method. For that case, I assume that it's not necessary.
I think that the previous code is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think try-with-resource is better since it ensures that server is closed even when client.deleteSavedQuery throws exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure.

@exoego exoego merged commit 4611f0c into master Jan 31, 2023
@exoego exoego deleted the try-with-resource branch January 31, 2023 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore internal changes (not user-facing)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants