Conversation
JustArchi
left a comment
JustArchi
left a comment
There was a problem hiding this comment.
New tests pass so LGTM
This comment has been minimized.
This comment has been minimized.
| /// </summary> | ||
| internal sealed class BlockingStream : Stream | ||
| { | ||
| private static readonly byte[] TestData = Encoding.UTF8.GetBytes( |
There was a problem hiding this comment.
IMO this would be better as a wrapper for an existing Stream than as a weird stream with hardcoded content.
|
|
||
| private long _backingPosition; | ||
|
|
||
| internal BlockingStream(int maxReadAtOnce = 1) |
There was a problem hiding this comment.
If maxReadAtOnce is customisable then BlockingStream isn't guaranteed to be blocking.
It looks like it's only guaranteed to be blocking when our maxReadAtOnce is smaller than the StreamReader's bufferSize, which is also configurable.
It may also be possible that if StreamReader's bufferSize is 1 then it will never block. 🤔
| // Never read more than user-configured limit | ||
| if (count > MaxReadAtOnce) count = MaxReadAtOnce; | ||
|
|
||
| for (var i = 0; i < count; i++) buffer[offset++] = TestData[_backingPosition++]; |
| internal class StreamsTestCase | ||
| { | ||
| [TestCase(1)] | ||
| [TestCase(4096)] |
There was a problem hiding this comment.
I don't think 4096 gets us as blocking stream, as per the previous comment. The default buffer size is 1024.
| } | ||
|
|
||
| var text = textReader.ReadLine(); | ||
| var text = sb.ToString(); |
There was a problem hiding this comment.
Why this loop? Is there something wrong with textReader.ReadLine() from the old code? If its the case that we might have something in our little peekedNext field, can we not deal with that in a simpler manner?
There was a problem hiding this comment.
I wonder if it would be simpler to build a TextReader wrapper.
There was a problem hiding this comment.
I initially wanted to just use Stream, but that had its own problems.
I think ReadLine may return just for \r without being followed by \n which isn't how Valve deals with it (always looking for \n only).
|
I moved |
ValveKeyValue/ValveKeyValue.Test/Text/CommentWithCrOnlyTestCase.cs
Outdated
Show resolved
Hide resolved
3 participants
Fixes #48
StreamReader.Peek() just returns -1 instead of blocking, we mitigate that by using Read() which does block.