fix: partial line handling in docker events listener#8
Conversation
Improves the handling of stdout data from the docker events process by buffering incomplete lines and only processing complete lines. This prevents errors when event data arrives in chunks that do not align with line boundaries.
skyrpex
force-pushed
the
partial-line-handling-in-docker-events-listener
branch
from
3f8328c to
068e717
Compare
|
I'll push a commit with a small refactor and some unit tests. |
tiurin
left a comment
tiurin
left a comment
There was a problem hiding this comment.
I only have one naming question, otherwise thanks for adding comprehensive test cases!
src/utils/jsonl-stream.ts
Outdated
| * Writable stream that buffers data until a newline, | ||
| * parses each line as JSON, and emits the parsed object. | ||
| */ | ||
| export class JsonlStream extends Writable { |
There was a problem hiding this comment.
nit: may be it's just for me, I find it hard to read this class name - lowercase l before uppercase S is a bit lost and indeed may look like a typo. :) Moreover, jsonl is not fully standardized yet (both JSONL and JSON Lines are used). How about JsonLineStream for both class and variable name? Again, had to use singular "Line" to avoid double S in JsonLinesStream, but I'm fine with both. WDYT?
| export class JsonlStream extends Writable { | |
| export class JsonLineStream extends Writable { |
There was a problem hiding this comment.
I'm also torn between JsonLineStream and JsonLinesStream, but I believe the latter is more correct. Asked AI for an opinion :)
JsonLinesStream is the better choice.
• The format is commonly called “JSON Lines” (see: jsonlines.org ↗), not “JSON Line”.
• The class processes multiple lines, not just a single line.
• “JsonLinesStream” clearly communicates that it deals with a stream of JSON lines.
So: JsonLinesStream is correct and idiomatic.
There was a problem hiding this comment.
Does AI have opinion on readability and accidental ligatures? :D
I do, and:
- I agree that plural is better semantically 👍
sSis more difficult to pronounce but easier to read thanlS, lowercasesis not lost.
Thanks for updating this!
There was a problem hiding this comment.
Agreed that it's awkward having an sS, although when it comes to pronouncing you can just omit it and say Json Line Stream, I won't notice the difference 😆
| assert.deepStrictEqual(callbackCalls[0], { key: "value1" }); | ||
| }); | ||
|
|
||
| test("should handle multiple messages in chunks", () => { |
There was a problem hiding this comment.
Thanks for adding this test case, it is exactly the doubt I had when looking at the code!
2 participants
Improves the handling of stdout data from the docker events process by buffering incomplete lines and only processing complete lines. This prevents errors when event data arrives in chunks that do not align with line boundaries.