Updated readInt method so streams are closed on failure or success#1476
Updated readInt method so streams are closed on failure or success#1476srnkan wants to merge 1 commit intoprocessing:mainfrom
Conversation
|
I'd like @Ebaron96 to be assigned as a reviewer if possible |
|
@srnkan sorry for the confusion in closing the issue without comment. i'll leave a review soon. i can't add ebaron96 as a reviewer until they have contributed code. but they are welcome to review. |
catilac
left a comment
There was a problem hiding this comment.
This is a great start! I noticed there aren't any tests in the UpdateCheckTest class.
It could use some!
| public class UpdateCheckTest { | ||
| } |
There was a problem hiding this comment.
This could use some tests!
There was a problem hiding this comment.
I agree, but I was unable to compile tests for the issue because of the ongoing testing issue. Once that issue is resolved, I'm happy to go back and add tests. Thank you!
There was a problem hiding this comment.
OH my gosh, yes. I just want to clarify my understanding of the testing issue, is it the broken tests for ProcessingPlugin ? Or something in addition to that?
There was a problem hiding this comment.
It's the same issue that Mark was talking about in the discord yesterday. Certain .gradlew tests fail automatically
There was a problem hiding this comment.
ohh yes, thanks for clarifying @srnkan! that isn't a blocker in this case. we'll just assume those tests are broken, so feel free to get started on this test. it could be helpful to toss in a little assertTrue(true) as a sanity check
| try(InputStream stream = url.openStream(); | ||
| InputStreamReader isr = new InputStreamReader(stream); | ||
| BufferedReader reader = new BufferedReader(isr); | ||
| return Integer.parseInt(reader.readLine()); | ||
| BufferedReader reader = new BufferedReader(isr)) { | ||
| return Integer.parseInt(reader.readLine()); | ||
| } |
There was a problem hiding this comment.
this is really cool! good use of try-with-resources.
✅ Title: clear and descriptive
✅ Resolves: Resolves #1416
✅ Changes: I adapted the preexisting readInt method to include a try-with-resources, which closes all "closeables" regardless if the code errors or runs smoothly. This stops memory leakage by always closing necessary streams.
✅ Tests: Tests pass the "./gradlew test" and I did not add any additional tests
✅ Checklist: Tests pass the "./gradlew test" tests and branch is up-to-date