Skip to content

Updated readInt method so streams are closed on failure or success #1476

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.

Sign up for GitHub

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

Open
srnkan wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Updated readInt method so streams are closed on failure or success #1476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions app/src/processing/app/UpdateCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,12 @@ protected boolean promptToOpenContributionManager() {

protected int readInt(String filename) throws IOException {
URL url = new URL(filename);
InputStream stream = url.openStream();
// try-with-resources auto closes things of type "Closeable" even the code throws an error
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());
}
Comment on lines +210 to +214
Copy link
Collaborator

@catilac catilac Mar 11, 2026

Choose a reason for hiding this comment

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

this is really cool! good use of try-with-resources.

}


Expand Down
4 changes: 4 additions & 0 deletions app/test/processing/app/UpdateCheckTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
package processing.app;

public class UpdateCheckTest {
}
Comment on lines +3 to +4
Copy link
Collaborator

@catilac catilac Mar 11, 2026

Choose a reason for hiding this comment

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

This could use some tests!

Copy link
Author

@srnkan srnkan Mar 11, 2026

Choose a reason for hiding this comment

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

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!

Copy link
Collaborator

@catilac catilac Mar 11, 2026

Choose a reason for hiding this comment

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

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?

Copy link

@Ebaron96 Ebaron96 Mar 11, 2026

Choose a reason for hiding this comment

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

looks good to me as well!

Copy link
Author

@srnkan srnkan Mar 11, 2026

Choose a reason for hiding this comment

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

It's the same issue that Mark was talking about in the discord yesterday. Certain .gradlew tests fail automatically

Copy link
Collaborator

@catilac catilac Mar 11, 2026

Choose a reason for hiding this comment

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

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