search

fgeller / kt

Kafka command line tool that likes JSON
MIT License
950 stars 100 forks source link

kt consume: stricter parsing of offsets #105

Closed rogpeppe closed 4 years ago

rogpeppe commented 4 years ago

Currently parseOffsets uses a regexp-based parser which is considerably more permissive than it probably should be. It will parse almost any string without error, even such "obviously wrong" specifications such as bogus and ::::.

We specify the grammar somewhat more formally and rewrite the parser so that it's not regexp-based, which allows us to give better error messages, is arguably more understandable and will catch user errors before we actually try to talk to Kafka.

This change is a prelude to a change that is intended to add timestamp-based search to kt. By using a more precise grammar specification, that change should become easier.

We also add some missing TestParseOffsets test cases to cover errors and some other corners of the grammar.

fgeller commented 4 years ago

Hi @rogpeppe - what a beautiful PR, thank you very much! I updated the tiny merge conflict and will pull the changes in! Reading through the changes, the only thoughts on changes that I had were on naming - so we could maybe try to use names that match those identifiers in the grammar. Would you be interested in reviewing tiny changes like that? I understand the delay on feedback was enormous, sorry for that! I've been a full time dad for the past 2.5 years and only resumed work on my Github projects a few days ago. Let me know if you'd be interested, especially as you mention possibly more work regarding timestamps?