Skip to content

Conversation

@elharo
Copy link
Contributor

@elharo elharo commented Feb 23, 2020

@elharo elharo requested a review from Tibor17 February 23, 2020 16:23
, text
, is( expectedContent ) );
try (Scanner base = new Scanner( expandedFile );
Scanner scanner = base.useDelimiter( "\n" ) )
Copy link
Contributor

Choose a reason for hiding this comment

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

Having two local variables with the same reference is not necessary. It is the flow design:
https://docs.oracle.com/javase/7/docs/api/java/util/Scanner.html#useDelimiter(java.util.regex.Pattern)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an interesting corner case I'd not encountered before and it took a while to figure out. It cures a warning about failure to close the first scanner in try-with-resources block. Though looking at it now I see another way to approach this.

Copy link
Contributor Author

@elharo elharo left a comment

Choose a reason for hiding this comment

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

PTAL

Copy link
Contributor Author

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Ping. Can I move forward with this one?

@rfscholte
Copy link
Contributor

@elharo
Copy link
Contributor Author

elharo commented Mar 21, 2020 via email

Copy link
Contributor Author

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Can I get a formal approval on this?

@elharo elharo merged commit 001baa7 into master Mar 23, 2020
@elharo elharo deleted the xml2 branch March 23, 2020 20:29
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.

4 participants