keichi / binary-parser

A blazing-fast declarative parser builder for binary data
MIT License
864 stars 134 forks source link

ParserOptions.readUntil is not consistent with ParserOptions.length and ParserOptions.lengthInBytes #167

Closed Rzial closed 3 years ago

Rzial commented 3 years ago

πŸ‘‹ I have found an inconsistency while using the readUntil option from ParserOptions as function.

When you use .length or .lengthInBytes as function you get the parsing context (the actual struct) on the this variable, however, with the readUntil option you get the parser definition since this is being passed instead ${ctx.generateVariable()} on the following ocurrences:

https://github.com/keichi/binary-parser/blob/master/lib/binary_parser.ts#L1032 https://github.com/keichi/binary-parser/blob/master/lib/binary_parser.ts#L1118 https://github.com/keichi/binary-parser/blob/master/lib/binary_parser.ts#L1210

Is there any reason to need the Parser definition and not get the context?

P.S.: The .readUntil option signature doesn't admit a function on TypeScript. We can fix it with the following change

-readUntil?: 'eof'; +readUntil?: 'eof' | ((item: any, buffer: Buffer) => boolean); or +readUntil?: 'eof' | ((item: any, buffer: Buffer) => any); if you would like like to admit falsy values as predicate

P.S.2: I can submit a PR with this changes which I already have done my local side if you are agree with it πŸ˜„ P.S.3: This also happens with the .formatter option.

keichi commented 3 years ago

Is there any reason to need the Parser definition and not get the context?

Hi, thanks for pointing this out. AFAIK there is no reason. I think we simply missed it. Can you open a PR?

P.S.: The .readUntil option signature doesn't admit a function on TypeScript. We can fix it with the following change

-readUntil?: 'eof'; +readUntil?: 'eof' | ((item: any, buffer: Buffer) => boolean); or +readUntil?: 'eof' | ((item: any, buffer: Buffer) => any); if you would like like to admit falsy values as predicate

P.S.2: I can submit a PR with this changes which I already have done my local side if you are agree with it πŸ˜„ P.S.3: This also happens with the .formatter option.

Yes I would very much appreciate it if you could submit another PR. The signatures are imperfect because we migrated this project from JavaScript to TypeScript at one point and missed several options back then. I know the type definition in DefinitelyTyped is more accurate but I haven't had the time to fix the signatures in this repo.

keichi commented 3 years ago

Addressed by #168