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

Use try with resources for file system #367

Merged
merged 1 commit into from
May 25, 2023

Conversation

asaarilahti
Copy link
Member

@asaarilahti asaarilahti commented May 24, 2023

Related to #332 - close file system also in case an exception is thrown. Unlikely to be an actual fix, but at least should rule out earlier failures as a cause.

Also switch from a visitor to a stream.

@asaarilahti asaarilahti requested a review from a team as a code owner May 24, 2023 10:29
@asaarilahti asaarilahti added this to the next milestone May 24, 2023
@cberg-zalando
Copy link
Member

Any reason for the switch from visitor to stream?

@asaarilahti
Copy link
Member Author

Any reason for the switch from visitor to stream?

Nothing major. I think the visitor is not that common nowadays, so streams might make the code easier to read.

Comment on lines +65 to +67
final Path jarPath = fileSystem.getPath(resourcePath);

try (Stream<Path> stream = Files.walk(jarPath)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
final Path jarPath = fileSystem.getPath(resourcePath);
try (Stream<Path> stream = Files.walk(jarPath)) {
try (Stream<Path> stream = Files.walk(fileSystem.getPath(resourcePath))) {

Copy link
Member

Choose a reason for hiding this comment

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

You don't use the variable later, so why actually assign it at all?!

Copy link
Member Author

Choose a reason for hiding this comment

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

You don't use the variable later, so why actually assign it at all?!

Would be nice, but it's actually used (in lines 72 and 77).

Copy link
Member

Choose a reason for hiding this comment

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

So much for my eyesight. 😏

@cberg-zalando cberg-zalando merged commit 8f9727a into master May 25, 2023
@cberg-zalando cberg-zalando deleted the always-close-file-system branch May 25, 2023 06:54
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