refactor: CLI subcommand structure#59
Conversation
📝 WalkthroughWalkthroughRefactored the CLI from a single Click command to a Click group with subcommands. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| # Filter out flags/options | ||
| pos_args = [a for a in args if not a.startswith("-")] | ||
| if len(pos_args) == 2: | ||
| ctx.invoke(compare, file_a=pos_args[0], file_b=pos_args[1]) |
There was a problem hiding this comment.
Bug: The legacy argument parser incorrectly includes option values (e.g., json from --format json) as positional arguments, causing the command to fail when options are used.
Severity: MEDIUM
Suggested Fix
To fix the legacy argument parsing, use a proper argument parser like argparse.ArgumentParser with parse_known_args(). This will correctly separate known options and their values from the positional arguments, ensuring that only the file paths are passed to ctx.invoke(compare, ...). This avoids the flawed logic of filtering arguments based on a startswith("-") check.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sounddiff/cli.py#L35-L38
Potential issue: The legacy invocation path in `main` uses a list comprehension `[a for
a in args if not a.startswith("-")]` to find positional arguments. This logic is flawed
because it cannot distinguish between a positional argument and a value associated with
an option (e.g., `json` in `--format json`). When a user runs the command with an option
that takes a value, such as `sounddiff file_a.wav file_b.wav --format json`, the
option's value is incorrectly counted as a positional argument. This causes the argument
count check `len(pos_args) == 2` to fail, preventing the comparison from running and
showing the help text instead.
Did we get this right? 👍 / 👎 to inform future reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: b04af491-7258-4221-8cb4-b7a11478aad2
📒 Files selected for processing (1)
src/sounddiff/cli.py
| if ctx.invoked_subcommand is None: | ||
| # Handle legacy positional arguments: sounddiff FILE_A FILE_B | ||
| # We only do this if exactly two arguments are provided and they look like paths. | ||
| args = sys.argv[1:] | ||
| # Filter out flags/options | ||
| pos_args = [a for a in args if not a.startswith("-")] | ||
| if len(pos_args) == 2: | ||
| ctx.invoke(compare, file_a=pos_args[0], file_b=pos_args[1]) | ||
| else: | ||
| click.echo(ctx.get_help()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that ctx.args is the correct mechanism for Click groups
# and check how the existing tests invoke the CLI
# Check Click documentation pattern for groups with leftover args
rg -n "ctx\.args" --type py
# See how tests currently invoke main
rg -nA10 "runner\.invoke" tests/Repository: systemblueteam/sounddiff
Length of output: 2789
Legacy fallback breaks with Click's CliRunner — sys.argv[1:] contains stale process arguments, not test inputs.
CliRunner.invoke() does not populate sys.argv; it passes arguments directly to Click's parser. The current code reads sys.argv[1:], which contains pytest's original invocation (e.g., ['pytest', 'tests/test_cli.py']), causing the positional-argument filter to fail. This breaks tests that rely on the legacy sounddiff FILE_A FILE_B syntax.
Use ctx.args to access unparsed positional arguments:
Proposed fix using ctx.args
`@click.group`(invoke_without_command=True)
`@click.version_option`(version=__version__, prog_name="sounddiff")
`@click.pass_context`
def main(ctx: click.Context) -> None:
"""Compare and inspect audio files.
sounddiff compare FILE_A FILE_B
sounddiff inspect FILE
"""
if ctx.invoked_subcommand is None:
- # Handle legacy positional arguments: sounddiff FILE_A FILE_B
- # We only do this if exactly two arguments are provided and they look like paths.
- args = sys.argv[1:]
- # Filter out flags/options
- pos_args = [a for a in args if not a.startswith("-")]
- if len(pos_args) == 2:
- ctx.invoke(compare, file_a=pos_args[0], file_b=pos_args[1])
+ # Handle legacy positional arguments: sounddiff FILE_A FILE_B
+ # ctx.args contains unparsed positional arguments after Click processing.
+ if len(ctx.args) == 2:
+ ctx.invoke(compare, file_a=ctx.args[0], file_b=ctx.args[1])
else:
click.echo(ctx.get_help())Note: This fix uses Click's internal argument parser (tested by CliRunner), not the process environment. Options passed alongside positional args (e.g., --format json) are not forwarded to compare; users must use explicit subcommand syntax (sounddiff compare FILE_A FILE_B --format json) for options.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ctx.invoked_subcommand is None: | |
| # Handle legacy positional arguments: sounddiff FILE_A FILE_B | |
| # We only do this if exactly two arguments are provided and they look like paths. | |
| args = sys.argv[1:] | |
| # Filter out flags/options | |
| pos_args = [a for a in args if not a.startswith("-")] | |
| if len(pos_args) == 2: | |
| ctx.invoke(compare, file_a=pos_args[0], file_b=pos_args[1]) | |
| else: | |
| click.echo(ctx.get_help()) | |
| if ctx.invoked_subcommand is None: | |
| # Handle legacy positional arguments: sounddiff FILE_A FILE_B | |
| # ctx.args contains unparsed positional arguments after Click processing. | |
| if len(ctx.args) == 2: | |
| ctx.invoke(compare, file_a=ctx.args[0], file_b=ctx.args[1]) | |
| else: | |
| click.echo(ctx.get_help()) |
There was a problem hiding this comment.
@systemblueteam, glad that aligns! Once you apply the ctx.args fix, the legacy fallback will work correctly both in production (real sys.argv) and under CliRunner in tests.
Quick reminder: with ctx.args, any options passed in legacy mode (e.g. sounddiff file_a.wav file_b.wav --format json) will not be forwarded to compare — only the two positional paths will be. Users needing extra options should use the explicit subcommand form (sounddiff compare file_a.wav file_b.wav --format json). Worth documenting that caveat somewhere visible (e.g. --help text or the migration notes).
(´• ω •`) 🐇 ~ carefully noting things down
✏️ Learnings added
Learnt from: systemblueteam
Repo: systemblueteam/sounddiff PR: 59
File: src/sounddiff/cli.py:31-40
Timestamp: 2026-03-27T09:12:56.694Z
Learning: In src/sounddiff/cli.py (Python, Click), the legacy fallback in the `main` Click group must read unparsed positional arguments from `ctx.args`, not `sys.argv[1:]`. `CliRunner` in tests does not populate `sys.argv`, so reading from `sys.argv` causes the legacy `sounddiff FILE_A FILE_B` path to silently fail during testing. The correct pattern is:
```python
if ctx.invoked_subcommand is None:
if len(ctx.args) == 2:
ctx.invoke(compare, file_a=ctx.args[0], file_b=ctx.args[1])
else:
click.echo(ctx.get_help())
```
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| # Filter out flags/options | ||
| pos_args = [a for a in args if not a.startswith("-")] | ||
| if len(pos_args) == 2: | ||
| ctx.invoke(compare, file_a=pos_args[0], file_b=pos_args[1]) |
There was a problem hiding this comment.
Legacy invocation silently ignores CLI options.
When invoking compare via the fallback, only file_a and file_b are passed. Options like --format, --verbose, and --no-color are not forwarded, so sounddiff file_a file_b --verbose would silently ignore --verbose.
If full backward compatibility is intended, consider parsing and forwarding the relevant options, or document that legacy mode only supports the two-file invocation without options.
There was a problem hiding this comment.
@systemblueteam, sounds good! Here's a minimal docstring update for main that documents the limitation and points users to compare:
`@click.group`(invoke_without_command=True)
`@click.version_option`(version=__version__, prog_name="sounddiff")
`@click.pass_context`
def main(ctx: click.Context) -> None:
"""Compare and inspect audio files.
sounddiff compare FILE_A FILE_B
sounddiff inspect FILE
Invoking sounddiff with two paths directly (legacy mode) is equivalent to
``sounddiff compare FILE_A FILE_B`` but ignores all options such as
--format, --verbose, and --no-color. Use the compare subcommand explicitly
for the full option set.
"""✏️ Learnings added
Learnt from: systemblueteam
Repo: systemblueteam/sounddiff PR: 59
File: src/sounddiff/cli.py:38-38
Timestamp: 2026-03-27T09:12:53.361Z
Learning: In src/sounddiff/cli.py, the legacy two-argument invocation (`sounddiff FILE_A FILE_B`) maps to the `compare` subcommand via `ctx.invoke` but intentionally does not forward CLI options (--format, --verbose, --no-color). This is by design; users who need those options should use `sounddiff compare FILE_A FILE_B` explicitly. The docstring of `main` documents this limitation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| @main.command() | ||
| @click.argument("file", type=click.Path(exists=True)) | ||
| def inspect(file: str) -> None: | ||
| """Inspect a single audio file's metadata and properties. | ||
|
|
||
| Displays duration, sample rate, channel count, bit depth, and format. | ||
| """ | ||
| try: | ||
| _, meta = load_audio(file) | ||
| click.echo(f"File: {meta.path}") | ||
| click.echo(f"Duration: {meta.duration:.3f}s") | ||
| click.echo(f"Sample Rate: {meta.sample_rate}Hz") | ||
| click.echo(f"Channels: {meta.channels}") | ||
| click.echo(f"Bit Depth: {meta.bit_depth or 'Unknown'}") | ||
| click.echo(f"Format: {meta.format_name}") | ||
| except Exception as e: | ||
| click.echo(f"Error: {e}", err=True) | ||
| sys.exit(1) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Well-implemented inspect subcommand.
Type annotations are complete, docstring follows Google style, and errors are properly caught and reported to stderr with exit code 1.
One consideration: the broad except Exception could mask unexpected errors (e.g., KeyboardInterrupt is not an Exception so that's fine, but MemoryError etc. would be caught). For a CLI tool this is generally acceptable to ensure clean user-facing errors, but you could consider catching more specific exceptions similar to the compare command's pattern (FileNotFoundError, ValueError, RuntimeError).
There was a problem hiding this comment.
@systemblueteam, here's the straightforward update to mirror the compare pattern:
`@main.command`()
`@click.argument`("file", type=click.Path(exists=True))
def inspect(file: str) -> None:
"""Inspect a single audio file's metadata and properties.
Displays duration, sample rate, channel count, bit depth, and format.
"""
try:
_, meta = load_audio(file)
click.echo(f"File: {meta.path}")
click.echo(f"Duration: {meta.duration:.3f}s")
click.echo(f"Sample Rate: {meta.sample_rate}Hz")
click.echo(f"Channels: {meta.channels}")
click.echo(f"Bit Depth: {meta.bit_depth or 'Unknown'}")
click.echo(f"Format: {meta.format_name}")
except FileNotFoundError as e:
click.echo(f"Error: {e}", err=True)
sys.exit(1)
except ValueError as e:
click.echo(f"Error: {e}", err=True)
sys.exit(1)
except RuntimeError as e:
click.echo(f"Error: {e}", err=True)
sys.exit(1)This keeps the two commands consistent and lets any truly unexpected errors (e.g., MemoryError, AttributeError from a bug) surface as tracebacks rather than being silently swallowed as a generic "Error: ..." message.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
systemblueteam
left a comment
systemblueteam
left a comment
There was a problem hiding this comment.
Nice start on the subcommand structure. Two things to fix:
The legacy fallback reads sys.argv directly, which won't work with Click's CliRunner and also grabs option values as positional args. Switch to ctx.args instead — that's what Click gives you for unparsed args in a group.
Also needs tests for the new paths: inspect, the legacy two-file invocation, and bare sounddiff showing help.
Small stuff: add a docstring note that legacy mode doesn't forward options (use sounddiff compare for those), and consider matching the specific exceptions from compare in inspect for consistency.
2 participants
Refactor
cli.pyto use a Click group withcompareandinspectsubcommands.Changes:
@click.group.comparesubcommand for audio comparison.inspectsubcommand for single-file metadata analysis.sounddiff FILE_A FILE_Busingctx.invoke.Fixes #54