You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Please see PR #16 in which I replaced the implementation of the input stream with something from the default Java API. I don't see why the home-made implementation is better, but it has a surprising effect, which is the reason I've raised this PR.
Why does the original version of the unit test work? The first scanner should, on nextLine, fast forward through the text to cache some of the line after? In the original code it doesn't, meaning that the test is able to pass. However, I think the test should not pass (it doesn't with this refactor) and the replacement test is more correct.
I also changed the README where I found it slightly hard to understand why you were asserting the second line, but it seemed simpler when we assert both first and second line.
This may or may not seem like an improvement. Just sharing.
The text was updated successfully, but these errors were encountered:
ashleyfrieze
changed the title
InputStream does not need to be home-made
Does InputStream need to be home-made?
Nov 14, 2020
I've found some use cases where the default implementation starts to struggle with my expectations... so I've written some tests in #18 that pass with the current implementation... but should they? Or should Scanner be hitting the exception thrown at the end of the stream earlier?
If the stream throws when it's asked to read beyond its end, then the tests in #18 would not all pass, because scanner reads ahead. For interest, merge both branches together to see what I mean.
Please see PR #16 in which I replaced the implementation of the input stream with something from the default Java API. I don't see why the home-made implementation is better, but it has a surprising effect, which is the reason I've raised this PR.
Why does the original version of the unit test work? The first scanner should, on
nextLine
, fast forward through the text to cache some of the line after? In the original code it doesn't, meaning that the test is able to pass. However, I think the test should not pass (it doesn't with this refactor) and the replacement test is more correct.I also changed the README where I found it slightly hard to understand why you were asserting the second line, but it seemed simpler when we assert both first and second line.
This may or may not seem like an improvement. Just sharing.
The text was updated successfully, but these errors were encountered: