Skip to content

Commit

Permalink
Ensure return code non-zero on cli error (#3499)
Browse files Browse the repository at this point in the history
After inspection, only the `shell_completion` did not use click methods
to report errors. Fixed and added/updated tests for both internally
detected errors.

## 📝 Summary

Fixes #3476

## 🔍 Description of Changes
After code inspection, only the `shell_completion` did not use click
methods to report errors. Fixed and added/updated tests for both
internally detected errors.

```bash
❯ pytest -p no:warnings tests/_cli/test_cli.py::test_shell_completion
===================================================== test session starts =====================================================
platform win32 -- Python 3.13.0, pytest-8.3.4, pluggy-1.5.0
rootdir: C:\Users\hwine\marimo
configfile: pyproject.toml
plugins: anyio-4.8.0
collected 6 items

tests\_cli\test_cli.py ......                                                                                            [100%]

===================================================== 6 passed in 14.19s ======================================================
```

## 📋 Checklist

- [x] I have read the [contributor
guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md).
- [ ] For large changes, or changes that affect the public API: this
change was discussed or approved through an issue, on
[Discord](https://marimo.io/discord?ref=pr), or the community
[discussions](https://github.com/marimo-team/marimo/discussions) (Please
provide a link if applicable).
- [x] I have added tests for the changes made.
- [x] I have run the code and verified that it works as expected.

## 📜 Reviewers

@mscolnick
  • Loading branch information
hwine authored Jan 18, 2025
1 parent b7d78be commit 1d64298
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 9 deletions.
12 changes: 5 additions & 7 deletions marimo/_cli/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -789,20 +789,19 @@ def env() -> None:


@main.command(
help="Install shell completions for marimo. Supports bash, zsh, fish, and elvish."
help="Install shell completions for marimo. Supports bash, zsh, and fish."
)
def shell_completion() -> None:
shell = os.environ.get("SHELL", "")
if not shell:
click.echo(
raise click.UsageError(
"Could not determine shell. Please set $SHELL environment variable.",
err=True,
)
return

# in case we're on a windows system, use .stem to remove extension
shell_name = Path(shell).stem

# N.B. change the help message above when changing supported shells
commands = {
"bash": (
'eval "$(_MARIMO_COMPLETE=bash_source marimo)"',
Expand All @@ -820,9 +819,8 @@ def shell_completion() -> None:

if shell_name not in commands:
supported = ", ".join(commands.keys())
click.echo(
f"Unsupported shell: {shell_name}. Supported shells: {supported}",
err=True,
raise click.UsageError(
f"Unsupported shell: {shell_name} (from $SHELL). Supported shells: {supported}",
)
return

Expand Down
4 changes: 2 additions & 2 deletions tests/_cli/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -818,8 +818,8 @@ def test_cli_run_sandbox_prompt_yes() -> None:
),
),
# invalid shell values, rc of 0, data only on stderr
# (N.B. rc will become 2 when Issue #3476 is fixed)
("bogus", 0, False, True),
("bogus", 2, False, True),
("", 2, False, True), # usage error displayed
],
)
def test_shell_completion(
Expand Down

0 comments on commit 1d64298

Please sign in to comment.