Skip to content

refactor: CLI subcommand structure #59

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Deepak8858 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: CLI subcommand structure #59

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 48 additions & 6 deletions src/sounddiff/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,35 @@

from sounddiff import __version__
from sounddiff.core import diff
from sounddiff.formats import FFMPEG_FORMATS
from sounddiff.formats import FFMPEG_FORMATS, load_audio
from sounddiff.report import render
from sounddiff.types import OutputFormat

_PROGRESS_THRESHOLD = 30


@click.command()
@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])
Comment on lines +35 to +38
Copy link
Copy Markdown

@sentry sentry bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

else:
click.echo(ctx.get_help())
Comment on lines +31 to +40
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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.

Suggested change
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())

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.



@main.command()
@click.argument("file_a", type=click.Path(exists=True))
@click.argument("file_b", type=click.Path(exists=True))
@click.option(
Expand Down Expand Up @@ -49,8 +70,7 @@
default=False,
help="Disable colored terminal output.",
)
@click.version_option(version=__version__, prog_name="sounddiff")
def main(
def compare(
file_a: str,
file_b: str,
output_format: str,
Expand All @@ -60,8 +80,6 @@ def main(
) -> None:
"""Compare two audio files and report what changed.

sounddiff FILE_A FILE_B

Compares FILE_A (reference) against FILE_B (comparison) and reports
differences in loudness, spectral content, timing, and potential issues.
"""
Expand Down Expand Up @@ -113,3 +131,27 @@ def main(
click.echo(f"Report written to {output_path}")
else:
click.echo(output)


@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)
Comment on lines +136 to +153
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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).

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.



if __name__ == "__main__":
main()
Loading