Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates bqtools to version 0.5.6, adding support for multiple input files in the info command and introducing a new --json output format. It also implements underscore-separated formatting for record counts in tabular views. The review feedback identifies a regression where error handling was omitted during block header iteration and suggests enforcing CLI flag conflicts for the num flag. Additionally, there are recommendations to improve performance by processing files lazily and to replace fragile file extension checks with the tool's internal format detection.
src/commands/info/mod.rs
Outdated
| )?; | ||
| let reader = cbq::MmapReader::new(path)?; | ||
| for header in reader.iter_block_headers() { | ||
| println!("{:?}", header); |
There was a problem hiding this comment.
The ? operator is missing when printing block headers. This causes the output to be wrapped in Ok(...) and prevents errors from being propagated, which is a regression from the previous implementation where header? was used to handle potential I/O errors during iteration.
| println!("{:?}", header); | |
| println!("{:?}", header?); |
src/cli/info.rs
Outdated
| #[clap(short, long)] | ||
| pub num: bool, |
There was a problem hiding this comment.
The num flag is implemented as mutually exclusive with other output modes in the run function (via the else if chain), but this constraint is not enforced in the CLI definition. This allows users to provide conflicting flags (e.g., --num --show-index) where only one will be honored without warning. Adding conflicts_with_all would improve the user experience and maintain consistency with the json flag definition.
| #[clap(short, long)] | |
| pub num: bool, | |
| #[clap(short, long, conflicts_with_all=["show_index", "show_headers"])] |
src/commands/info/mod.rs
Outdated
| let all_info: Vec<BinseqInfo> = args | ||
| .input | ||
| .iter() | ||
| .map(|path| BinseqInfo::from_path(path.as_str())) | ||
| .collect::<Result<_>>()?; |
There was a problem hiding this comment.
The all_info vector is eagerly populated for all input files at the start of the function. This is inefficient when --show-headers is used (as it doesn't use all_info) or when processing many files in non-JSON modes (as it delays output and increases memory usage by holding all indices in memory). Consider processing files lazily in a loop unless the --json flag is specifically requested, which requires the full collection for a single array output.
src/commands/info/mod.rs
Outdated
| } | ||
| } else if args.opts.show_headers { | ||
| for path in args.input.iter() { | ||
| if !path.ends_with(".cbq") { |
There was a problem hiding this comment.
Using ends_with(".cbq") to verify the file format is fragile as it is case-sensitive and fails for files without the expected extension (e.g., .CBQ). Since the tool already detects the format via BinseqReader during the all_info collection, you should use the detected variant instead of relying on the filename suffix.
1 participant
No description provided.