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

Website only: Update first example to use try with resources #3096

Merged
merged 1 commit into from
Jan 9, 2017

Conversation

rbygrave
Copy link
Contributor

@rbygrave rbygrave commented Jan 9, 2017

This changes the first example on the website from using try/finally to using try with resources.

This fixes 2 issues with my previous PR for the first website example.

  • The actual linked GetExample.java uses try with resources (not try finally so my change didn't match the linked GetExample.java)
  • My try/finally example had issues. It didn't initialise the Response properly and close if non-null etc

Apologies for getting this wrong in the first PR. I think this fixes those 2 issues in my previous PR and I think it is nice if the first example code seen is as simple and clean (and correct) as possible (I think try with resources is a bit cleaner/nicer than try/finally).

Copy link
Collaborator

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

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

One small problem with this is that most of our users are building with tools that don't worry try with resources. But it's still an improvement over broken code.

@swankjesse swankjesse merged commit 592d53b into square:master Jan 9, 2017
@rbygrave
Copy link
Contributor Author

rbygrave commented Jan 9, 2017

users are building with tools that don't worry try with resources

Yes. I'll push up a separate 3rd example with a separate try/finally example. It uses Util.closeQuietly() which you may consider internal and wish to discourage (so maybe this isn't quite right) but I'll put it up for review.

@swankjesse
Copy link
Collaborator

Yep. closeQuietly is in internal and not for our users!

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