Handle conflicting --show-artifacts and --no-artifacts flags#21
Handle conflicting --show-artifacts and --no-artifacts flags#21Riyaa-Bajpai wants to merge 2 commits intomahimailabs:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds validation to the scan CLI command to detect and reject conflicting artifact flags (--show-artifacts with --no-artifacts). When both flags are provided simultaneously, the command prints an error message and exits with code 1. Includes a test function to verify this error handling behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/envoic/cli.py`:
- Around line 382-388: The test function test_conflicting_artifact_flags_error
should be moved out of production module and into a proper test file (e.g.,
tests/test_cli.py); in that test file import typer.testing.CliRunner and the app
symbol from src.envoic.cli, create a CliRunner instance (explicitly set
mix_stderr=True or False), invoke the app with ["scan", "--show-artifacts",
"--no-artifacts"], and assert non-zero exit_code and that the error text is
checked against result.stderr if you set mix_stderr=False (or result.output if
you keep mix_stderr=True); finally remove the test function from
src/envoic/cli.py to avoid runtime NameError from undefined runner.
- Around line 177-182: The indentation around the artifact-guard is wrong and
the check runs (or causes a syntax error) after the scan; move and fix it so the
condition if show_artifacts and not include_artifacts: encloses both
typer.echo(...) and raise typer.Exit(code=1), and relocate this entire if-block
to run before calling _build_scan_result() so the CLI doesn't perform the full
filesystem scan when the incompatible flags are provided; ensure the variables
show_artifacts and include_artifacts are used exactly as named and that the
typer.echo and raise typer.Exit are indented to be inside that if block.
| if show_artifacts and not include_artifacts: | ||
| typer.echo( | ||
| "Error: --show-artifacts cannot be used together with --no-artifacts.", | ||
| err=True, | ||
| ) | ||
| raise typer.Exit(code=1) |
There was a problem hiding this comment.
Critical indentation bug — every scan invocation is broken
The typer.echo(...) and raise typer.Exit(code=1) lines are not indented inside the if block. Python raises an IndentationError here (confirmed by the Ruff invalid-syntax hint), so the module cannot even be imported and the entire CLI is non-functional.
Even if the indentation were accepted, both statements would execute unconditionally — every call to scan would print the error and exit with code 1.
Additionally, the guard fires after _build_scan_result() has already completed a full filesystem scan. Moving it above the scan call avoids wasted work.
🐛 Proposed fix
) -> None:
"""Scan a filesystem path for Python environments."""
+ if show_artifacts and not include_artifacts:
+ typer.echo(
+ "Error: --show-artifacts cannot be used together with --no-artifacts.",
+ err=True,
+ )
+ raise typer.Exit(code=1)
+
result = _build_scan_result(
path,
depth,
deep=deep,
stale_days=stale_days,
include_dotenv=include_dotenv,
include_artifacts=include_artifacts,
)
- if show_artifacts and not include_artifacts:
- typer.echo(
- "Error: --show-artifacts cannot be used together with --no-artifacts.",
- err=True,
- )
- raise typer.Exit(code=1)🧰 Tools
🪛 Ruff (0.15.1)
[warning] 178-178: Expected an indented block after if statement
(invalid-syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/envoic/cli.py` around lines 177 - 182, The indentation around the
artifact-guard is wrong and the check runs (or causes a syntax error) after the
scan; move and fix it so the condition if show_artifacts and not
include_artifacts: encloses both typer.echo(...) and raise typer.Exit(code=1),
and relocate this entire if-block to run before calling _build_scan_result() so
the CLI doesn't perform the full filesystem scan when the incompatible flags are
provided; ensure the variables show_artifacts and include_artifacts are used
exactly as named and that the typer.echo and raise typer.Exit are indented to be
inside that if block.
| def test_conflicting_artifact_flags_error(): | ||
| result = runner.invoke( | ||
| app, | ||
| ["scan", "--show-artifacts", "--no-artifacts"], | ||
| ) | ||
| assert result.exit_code != 0 | ||
| assert "cannot be used together" in result.output.lower() |
There was a problem hiding this comment.
runner is undefined — NameError at test runtime; test belongs in a dedicated test file
Three issues with this test:
-
NameError:runneris referenced on line 383 but is never imported or instantiated incli.py. The test will raiseNameError: name 'runner' is not definedimmediately. Atyper.testing.CliRunnerinstance must be created (and typically the import added). -
Test in production module:
test_conflicting_artifact_flags_erroris defined directly incli.py. This couples test infrastructure to production code and is non-idiomatic. It should live in a dedicated test file (e.g.,tests/test_cli.py). -
Stderr vs stdout assertion: The error is emitted with
err=True(stderr). The assertion checksresult.output(stdout). This works only when theCliRunneris created withmix_stderr=True(the default), but is fragile and should be explicit — or the assertion should useresult.stderrwhen the runner is configured withmix_stderr=False.
🛠️ Proposed fix (move to a test file)
In tests/test_cli.py:
+from typer.testing import CliRunner
+from envoic.cli import app
+
+runner = CliRunner()
+
+
+def test_conflicting_artifact_flags_error():
+ result = runner.invoke(
+ app,
+ ["scan", "--show-artifacts", "--no-artifacts"],
+ )
+ assert result.exit_code != 0
+ assert "cannot be used together" in result.output🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/envoic/cli.py` around lines 382 - 388, The test function
test_conflicting_artifact_flags_error should be moved out of production module
and into a proper test file (e.g., tests/test_cli.py); in that test file import
typer.testing.CliRunner and the app symbol from src.envoic.cli, create a
CliRunner instance (explicitly set mix_stderr=True or False), invoke the app
with ["scan", "--show-artifacts", "--no-artifacts"], and assert non-zero
exit_code and that the error text is checked against result.stderr if you set
mix_stderr=False (or result.output if you keep mix_stderr=True); finally remove
the test function from src/envoic/cli.py to avoid runtime NameError from
undefined runner.
mahimairaja
left a comment
mahimairaja
left a comment
There was a problem hiding this comment.
Please check the comments and update the PR
| include_artifacts=include_artifacts, | ||
| ) | ||
| if show_artifacts and not include_artifacts: | ||
| typer.echo( |
| yes=yes, | ||
| ) | ||
|
|
||
| def test_conflicting_artifact_flags_error(): |
There was a problem hiding this comment.
@Riyaa-Bajpai move the test to /tests directory
2 participants
This PR fixes the behavior when --show-artifacts and --no-artifacts
are provided together. The CLI now exits with a clear error message
instead of producing empty artifact output.
A test has been added to cover this scenario.
Summary by CodeRabbit
New Features
--show-artifactsand--no-artifactscannot be used simultaneously and displays a clear error message.Tests