Use new 'PeekableStream' to do binary categorization. #2753
Conversation
scottoneil-ms
requested review from
michaelcfanning,
suvamM,
shaopeng-gh,
rwoll,
yongyan-gh,
LingZhou-gh,
EasyRhinoMSFT and
cfaucon
as code owners
moving files around
scottoneil-ms
force-pushed
the
users/scott/peekable
branch
from
b85f123
to
016330b
Compare
| public class PeekableStreamTests | ||
| { | ||
| [Fact] | ||
| public void PeekableStream_BoundaryRewinds() |
There was a problem hiding this comment.
I think this covers a lot, but I've thought of some other cases to hit and you added one as well.
|
Release note, please! For this excellent change. |
|
I know the class is internal and we're swapping out for function that should be equivalent. We'll want signal in the release notes of when we made the switch-over though, in case users report an issue in the enumerated artifact class. In reply to: 1885935208 |
| throw new System.NotImplementedException(); | ||
| } | ||
|
|
||
| public override int Read(byte[] buffer, int offset, int count) |
There was a problem hiding this comment.
ok! I think I understand. your peekable stream is kinda like a pocket knife with a 'stop' before it fully opens. users can read the header bytes and you will 'stop' when you hit the transition point that will require you to abandon the rewind buffer. this gives the caller the change to easily fully inspect the header without worrying about an over-read. At this point, either the caller presumably rewinds (to get the entire stream by passing it to another reader) OR moves forward.
Two things:
- this is weird, but for readability, I might have ordered this code to reflect these three states reflecting where we are in the stream, 1) within the rewind buffer, 2) at the transition point, 3) past the transition point.
Simple because this makes it easier to build a mental model of what's going on by reading your commented code.
Second thing, consider a large comment at the top that describes the general approach. For fun, ask Copilot to generate it. :) haha.
There was a problem hiding this comment.
The "stop" point is more an implementation detail to make implementation easier than an API feature. The caller is in control of the buffer size. That's 100% of what they need to know to use the class correctly. I would not want them forming dependencies on the partial read behavior.
What the partial reads do, is let me operate over one byte range per Read() call. That simplifies things a bit. It would be harder to read and get correct (I claim) if I got hung up on delivering all the requested bytes, and had to compose both an internal buffer read and a call into the underlying stream, in the same call.
PeekableStream offers a very weak Seek(Origin) capability that expires once a configurable amount of Stream has been read.