fix(ts): support PhysicalLevelTechnique.NONE for integer streams#1138
fix(ts): support PhysicalLevelTechnique.NONE for integer streams#1138DoFabien wants to merge 2 commits into
Conversation
|
|
||
| /** | ||
| * Encodes unsigned BigInt64 values with varint compression (no zigzag) | ||
| * Encodes unsigned BigInt64 values as plain little-endian uint64 words. |
There was a problem hiding this comment.
Probably better to remove single line methods as it doesn't add a lot of value I believe...
| return buffer; | ||
| } | ||
|
|
||
| export function encodeUint32sLE(values: Uint32Array): Uint8Array { |
There was a problem hiding this comment.
Is this a simple copy of the data? If so maybe it's better to rename the method as copyArray, or simply remove it and use the content of the method when needed.
| const byteOffset = offset.get(); | ||
| values = new Int32Array(data.buffer, data.byteOffset + byteOffset, 4); | ||
| const decoded = decodeUint32sLE(data, offset, 4); | ||
| values = new Int32Array(decoded.buffer, decoded.byteOffset, decoded.length); |
There was a problem hiding this comment.
Can this be written like in the previous if statement as values = new Int32Array(decodeUint32sLE(data, offset, 4));?
Or maybe even something like:
let values: Int32Array = streamMetadata.physicalLevelTechnique === PhysicalLevelTechnique.VARINT
? decodeVarintInt32(data, offset, 4)
: decodeUint32sLE(data, offset, 4);
| } | ||
|
|
||
| export function encodeUint64sLE(values: BigUint64Array): Uint8Array { | ||
| return new Uint8Array(values.slice().buffer); |
| expect(result).toEqual(expectedValues); | ||
| }); | ||
|
|
||
| it("should decode little-endian signed Int64 words with PhysicalLevelTechnique.NONE", () => { |
There was a problem hiding this comment.
This test is not clear how the data is transformed from 0x0f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 to -8n.
I advise to add an encoderMethod to bridge this gap.
|
|
||
| it("should decode little-endian unsigned Int64 words with PhysicalLevelTechnique.NONE", () => { | ||
| const metadata = createInt64PhysicalNoneMetadata(1); | ||
| const data = new Uint8Array([0x88, 0x77, 0x66, 0x55, 0x44, 0x33, 0x22, 0x11]); |
| expect(offset.get()).toBe(16); | ||
| }); | ||
|
|
||
| it("should decode uint64 values from little-endian bytes", () => { |
| return fb; | ||
| } | ||
|
|
||
| export function decodeUint32sLE( |
There was a problem hiding this comment.
Other methods in this file has support for nullabilityBuffer, do the new methods require this support as well?
Also other methods are not throwing exceptions, so I think it's better to keep it without exception handling at this level.
|
Might be worth waiting for a decision about the issue you opened before continuing with the implementation. |
|
Thanks again for all the hard work on this! |
7ec23f0 to
a942f68
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1138 +/- ##
===========================================
+ Coverage 64.58% 96.02% +31.43%
===========================================
Files 93 68 -25
Lines 15606 4929 -10677
Branches 0 776 +776
===========================================
- Hits 10079 4733 -5346
+ Misses 5527 182 -5345
- Partials 0 14 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
This PR fixes TypeScript support for
PhysicalLevelTechnique.NONEon integer streams.The previous implementation did not read or write integer payloads as explicit little-endian words. In practice:
NONEpath decoded bytes incorrectlyNONEChanges
NONEpayloads as little-endian integer wordsNONEpayloads as little-endian integer wordsNONEsupport to the TypeScriptint64integer stream decode pathsInt32/Int64helpersphysicalLevelTechnique: NONENote
The repository is currently inconsistent on this topic:
docs/specification.mdanddocs/encodings.mddo not describe the same storyNONEI will open a separate issue in the next few hours to clarify the intended project-wide direction for the spec and the other implementations.