feat: Improve error messages for missing or invalid files#51
feat: Improve error messages for missing or invalid files#51HuiNeng6 wants to merge 1 commit intosystemblueteam:mainfrom
Conversation
- Replace Click default path validation with custom messages - Show helpful context when file not found (working directory) - Display tip when first file exists but second is missing - Add warning for non-audio file extensions Fixes: systemblueteam#50
📝 WalkthroughWalkthroughThe changes replace Click's default path validation with custom Rich-based validation logic, enabling more informative error messages that include parent directory context and audio file extension checking before processing begins. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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}
| Compares FILE_A (reference) against FILE_B (comparison) and reports | ||
| differences in loudness, spectral content, timing, and potential issues. | ||
| """ | ||
| console = Console() |
There was a problem hiding this comment.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7aa514ed-e1be-4606-96b1-ebea2378c363
📒 Files selected for processing (1)
src/sounddiff/cli.py
| @click.argument("file_a", type=click.Path()) | ||
| @click.argument("file_b", type=click.Path()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file structure and read the cli.py file
cat -n src/sounddiff/cli.py | head -100Repository: 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 -5Repository: systemblueteam/sounddiff
Length of output: 238
🏁 Script executed:
# Get the full cli.py file to understand the actual implementation
wc -l src/sounddiff/cli.pyRepository: 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.pyRepository: 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}")
EOFRepository: 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'}")
EOFRepository: 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 = filesAlso 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).
| Compares FILE_A (reference) against FILE_B (comparison) and reports | ||
| differences in loudness, spectral content, timing, and potential issues. | ||
| """ | ||
| console = Console() |
There was a problem hiding this comment.
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.
| console = Console() | |
| console = Console(stderr=True, no_color=no_color) |
| 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) |
There was a problem hiding this comment.
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.
| 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) |
| 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]") | ||
|
|
There was a problem hiding this comment.
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.
| 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]") |
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.
-
Console() on line 68 needs no_color=no_color. Line 117 already does this.
-
path.exists() passes for directories. Use path.is_file() instead.
-
The audio_extensions set is missing .opus. Use NATIVE_FORMATS | FFMPEG_FORMATS from formats.py instead of hardcoding.
2 participants
Improve CLI error messages for better user experience.
Changes:
Fixes: #50