Skip to content

feat: Improve error messages for missing or invalid files #51

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
HuiNeng6 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Improve error messages for missing or invalid files #51

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
29 changes: 27 additions & 2 deletions src/sounddiff/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@


@click.command()
@click.argument("file_a", type=click.Path(exists=True))
@click.argument("file_b", type=click.Path(exists=True))
@click.argument("file_a", type=click.Path())
@click.argument("file_b", type=click.Path())
Comment on lines +23 to +24
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.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's check the file structure and read the cli.py file
cat -n src/sounddiff/cli.py | head -100

Repository: systemblueteam/sounddiff

Length of output: 3944

🏁 Script executed:

# Check if there's a Click version specified in dependencies
fd -type f -name "*.txt" -o -name "*.toml" -o -name "*.cfg" | xargs grep -l "click" 2>/dev/null | head -5

Repository: systemblueteam/sounddiff

Length of output: 238

🏁 Script executed:

# Get the full cli.py file to understand the actual implementation
wc -l src/sounddiff/cli.py

Repository: systemblueteam/sounddiff

Length of output: 92

🏁 Script executed:

# Check formats.py to verify the claim about NATIVE_FORMATS and FFMPEG_FORMATS
cat -n src/sounddiff/formats.py

Repository: systemblueteam/sounddiff

Length of output: 5514

🏁 Script executed:

# Verify Click's actual behavior with argument arity - create a test script
python3 << 'EOF'
import click

`@click.command`()
`@click.argument`("file_a", type=click.Path())
`@click.argument`("file_b", type=click.Path())
def test_cmd(file_a, file_b):
    """Test command with two required args."""
    print(f"Called with: {file_a}, {file_b}")

# Test what Click does with missing second argument
import sys
sys.argv = ["test", "only_one_file.txt"]
try:
    test_cmd()
except SystemExit as e:
    print(f"Click exited with code: {e.code}")
EOF

Repository: systemblueteam/sounddiff

Length of output: 209

🏁 Script executed:

# Verify the Console() call actually needs stderr=True and no_color parameter
# by checking Click/Rich documentation patterns
python3 << 'EOF'
from rich.console import Console

# Test what happens when we pass no_color to Console
c1 = Console(no_color=True)
print(f"Console with no_color=True created: {c1}")

# Test stderr output
c2 = Console(stderr=True, no_color=False)
print(f"Console with stderr=True created: {c2}")

# Standard console (what current code does)
c3 = Console()
print(f"Standard Console file attribute: {c3.file}")
print(f"Is stderr? {c3.file.name if hasattr(c3.file, 'name') else 'not a tty'}")
EOF

Repository: systemblueteam/sounddiff

Length of output: 300

Click validates argument arity before calling main(), so single-file invocation won't reach your custom error message.

With two required click.argument decorators, Click exits with Missing argument 'FILE_B' before the function runs. To provide a custom error message for the single-file case (issue #50), change to variable args and validate in the function:

Suggested fix 
-@click.argument("file_a", type=click.Path())
-@click.argument("file_b", type=click.Path())
+@click.argument("files", nargs=-1, type=click.Path())
 ...
 def main(
-    file_a: str,
-    file_b: str,
+    files: tuple[str, ...],
     output_format: str,
     output_path: str | None,
     verbose: bool,
     no_color: bool,
 ) -> 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.
     """
-    console = Console()
+    console = Console(stderr=True, no_color=no_color)
+    if len(files) != 2:
+        got = ", ".join(files) if files else "<none>"
+        console.print("[red]Error:[/red] sounddiff needs two files to compare.")
+        console.print(f"[dim]Got: {got}[/dim]")
+        sys.exit(1)
+    file_a, file_b = files

Also fix the exists() validation to reject directories (file_path.is_file() instead of file_path.exists()), and add .opus to the hardcoded extensions (currently in FFMPEG_FORMATS but missing from the local list on line 87).

@click.option(
"--format",
"output_format",
Expand Down Expand Up @@ -65,6 +65,31 @@ def main(
Compares FILE_A (reference) against FILE_B (comparison) and reports
differences in loudness, spectral content, timing, and potential issues.
"""
console = Console()
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 Console object at line 68 is created without the no_color parameter, causing validation messages to be colored even when --no-color is specified.
Severity: MEDIUM

Suggested Fix

Pass the no_color parameter when instantiating the Console object. Change console = Console() to console = Console(no_color=no_color) on line 68 to respect the user's command-line flag.

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#L68

Potential issue: The `Console` object is instantiated on line 68 without passing the
`no_color` flag, which is available in the `main` function's scope. As a result, when a
user runs the command-line tool with `--no-color`, the new validation messages on lines
73-91 will still be printed with Rich color markup (e.g., `[red]`, `[yellow]`). This is
inconsistent with other `Console` instantiations in the codebase that correctly respect
the `no_color` flag, leading to unexpected color codes in logs or terminals that do not
support them.

Did we get this right? 👍 / 👎 to inform future reviews.

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.

⚠️ Potential issue | 🟠 Major

Validation messages should use stderr and honor --no-color.

Console() defaults to stdout and color enabled. This can pollute machine-readable stdout (e.g., --format json) and ignores the user’s --no-color flag for early validation output.

Proposed fix 
-    console = Console()
+    console = Console(stderr=True, no_color=no_color)
📝 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
console = Console()
console = Console(stderr=True, no_color=no_color)


# Validate file_a
path_a = pathlib.Path(file_a)
if not path_a.exists():
console.print(f"[red]Error:[/red] File not found: [bold]{file_a}[/bold]")
console.print(f"[dim]Looked in: {path_a.parent.absolute()}[/dim]")
console.print("[dim]Tip: Check the file path and make sure the file exists.[/dim]")
sys.exit(1)

# Validate file_b
path_b = pathlib.Path(file_b)
if not path_b.exists():
console.print(f"[red]Error:[/red] sounddiff needs two files to compare.")
console.print(f"[dim]Got: {file_a} (exists), but {file_b} not found.[/dim]")
console.print(f"[dim]Looked in: {path_b.parent.absolute()}[/dim]")
sys.exit(1)
Comment on lines +71 to +84
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.

⚠️ Potential issue | 🟠 Major

Existence-only checks miss invalid path types (e.g., directories).

Path.exists() passes for directories, so invalid inputs can fall through and fail later with less friendly exceptions. Validate that each path is a regular file before continuing.

Proposed fix 
     path_a = pathlib.Path(file_a)
     if not path_a.exists():
         console.print(f"[red]Error:[/red] File not found: [bold]{file_a}[/bold]")
         console.print(f"[dim]Looked in: {path_a.parent.absolute()}[/dim]")
         console.print("[dim]Tip: Check the file path and make sure the file exists.[/dim]")
         sys.exit(1)
+    if not path_a.is_file():
+        console.print(f"[red]Error:[/red] Not a file: [bold]{file_a}[/bold]")
+        sys.exit(1)

     path_b = pathlib.Path(file_b)
     if not path_b.exists():
         console.print(f"[red]Error:[/red] sounddiff needs two files to compare.")
         console.print(f"[dim]Got: {file_a} (exists), but {file_b} not found.[/dim]")
         console.print(f"[dim]Looked in: {path_b.parent.absolute()}[/dim]")
         sys.exit(1)
+    if not path_b.is_file():
+        console.print(f"[red]Error:[/red] Not a file: [bold]{file_b}[/bold]")
+        sys.exit(1)

As per coding guidelines, src/sounddiff/cli.py: "No unhandled exceptions reaching the user".

📝 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
path_a = pathlib.Path(file_a)
if not path_a.exists():
console.print(f"[red]Error:[/red] File not found: [bold]{file_a}[/bold]")
console.print(f"[dim]Looked in: {path_a.parent.absolute()}[/dim]")
console.print("[dim]Tip: Check the file path and make sure the file exists.[/dim]")
sys.exit(1)
# Validate file_b
path_b = pathlib.Path(file_b)
if not path_b.exists():
console.print(f"[red]Error:[/red] sounddiff needs two files to compare.")
console.print(f"[dim]Got: {file_a} (exists), but {file_b} not found.[/dim]")
console.print(f"[dim]Looked in: {path_b.parent.absolute()}[/dim]")
sys.exit(1)
path_a = pathlib.Path(file_a)
if not path_a.exists():
console.print(f"[red]Error:[/red] File not found: [bold]{file_a}[/bold]")
console.print(f"[dim]Looked in: {path_a.parent.absolute()}[/dim]")
console.print("[dim]Tip: Check the file path and make sure the file exists.[/dim]")
sys.exit(1)
if not path_a.is_file():
console.print(f"[red]Error:[/red] Not a file: [bold]{file_a}[/bold]")
sys.exit(1)
# Validate file_b
path_b = pathlib.Path(file_b)
if not path_b.exists():
console.print(f"[red]Error:[/red] sounddiff needs two files to compare.")
console.print(f"[dim]Got: {file_a} (exists), but {file_b} not found.[/dim]")
console.print(f"[dim]Looked in: {path_b.parent.absolute()}[/dim]")
sys.exit(1)
if not path_b.is_file():
console.print(f"[red]Error:[/red] Not a file: [bold]{file_b}[/bold]")
sys.exit(1)


# Validate audio file extensions
audio_extensions = {'.wav', '.mp3', '.flac', '.ogg', '.m4a', '.aac', '.wma', '.aiff', '.aif'}
for file_path, arg_name in [(path_a, 'FILE_A'), (path_b, 'FILE_B')]:
if file_path.suffix.lower() not in audio_extensions:
console.print(f"[yellow]Warning:[/yellow] {arg_name} ('{file_path.name}') may not be an audio file.")
console.print(f"[dim]Expected audio formats: {', '.join(sorted(audio_extensions))}[/dim]")

Comment on lines +87 to +92
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.

⚠️ Potential issue | 🟡 Minor

Hardcoded extension list is already stale (.opus false warning).

This duplicates format definitions and drifts from src/sounddiff/formats.py; currently .opus is supported there but warned as “may not be audio” here.

Proposed fix 
-from sounddiff.formats import FFMPEG_FORMATS
+from sounddiff.formats import FFMPEG_FORMATS, NATIVE_FORMATS
...
-    audio_extensions = {'.wav', '.mp3', '.flac', '.ogg', '.m4a', '.aac', '.wma', '.aiff', '.aif'}
+    audio_extensions = NATIVE_FORMATS | FFMPEG_FORMATS
📝 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
audio_extensions = {'.wav', '.mp3', '.flac', '.ogg', '.m4a', '.aac', '.wma', '.aiff', '.aif'}
for file_path, arg_name in [(path_a, 'FILE_A'), (path_b, 'FILE_B')]:
if file_path.suffix.lower() not in audio_extensions:
console.print(f"[yellow]Warning:[/yellow] {arg_name} ('{file_path.name}') may not be an audio file.")
console.print(f"[dim]Expected audio formats: {', '.join(sorted(audio_extensions))}[/dim]")
audio_extensions = NATIVE_FORMATS | FFMPEG_FORMATS
for file_path, arg_name in [(path_a, 'FILE_A'), (path_b, 'FILE_B')]:
if file_path.suffix.lower() not in audio_extensions:
console.print(f"[yellow]Warning:[/yellow] {arg_name} ('{file_path.name}') may not be an audio file.")
console.print(f"[dim]Expected audio formats: {', '.join(sorted(audio_extensions))}[/dim]")

# Best-effort duration probe: if sf.info() fails for either file,
# skip the progress bar and let diff() raise the proper error message.
show_progress = False
Expand Down