Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #177 +/- ##
==========================================
+ Coverage 82.25% 82.94% +0.68%
==========================================
Files 34 34
Lines 3652 3605 -47
==========================================
- Hits 3004 2990 -14
+ Misses 648 615 -33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
yunzheng
left a comment
yunzheng
left a comment
There was a problem hiding this comment.
I was about to ask for a concrete use case example for this flag, but looking at the unit tests, I see this is to not return an error if the input file is empty (0 bytes, or just newline)?. Is that correct?
If that is the case, I wonder if this should be the default or not, And then maybe introduce --strict or --fail-on-empty. (/cc @Schamper)
Also this seems to be a corner case, as it doesn't raise "RecordAdapterNotFound":
$ touch empty.records
$ rdump empty.records --ignore-empty-record-stream
2025-08-11 09:31:15,892 ERROR rdump encountered a fatal error: Unknown file format, not a RecordStream
$ echo $?
1|
|
||
| except Exception as e: | ||
| print_error(e) | ||
| if args.ignore_empty_record_stream and count == 0 and isinstance(e, RecordAdapterNotFound): |
There was a problem hiding this comment.
Why also RecordAdapterNotFound in the condition?
| except Exception as e: | ||
| print_error(e) | ||
| if args.ignore_empty_record_stream and count == 0 and isinstance(e, RecordAdapterNotFound): | ||
| log.warning("No records were read from the input stream") |
There was a problem hiding this comment.
If someone would use a selector that yields no records this warning would not be technically correct. As records have ben read from input stream but it yielded no records due to the selector.
I was thinking about adding extra return data to record_stream for how many records have been read (also for improved progress bar), but the selector is usually applied in the adapter so record_stream has no telemetry on this. Needs some more thought on how this could be achieved though and better for another PR.
There was a problem hiding this comment.
processed records are now tracked via an AppContext, see #184
I also was wondering if it's not better to swap the logic. Make it exit "cleanly" on empty streams by default, and only 1 if you wish it to fail on empty streams. |
|
The #203 PR makes empty record stream not fail by default (it does log a warning), which after consideration is a good default. So the question is now if it's still useful to have a CLI flag to error on empty record streams. |
|
Added the last test that it exits with an error code when the input is malformed. Should now cover all the tests that were introduced here. |
3 participants
This PR adds the flag
--ignore-empty-record-streamto therdumptool. This can be useful when a record stream does not yield any records and the desired exit code is zero.