Conversation
hiverge-robot
added
needs-triage
There was a problem hiding this comment.
Pull request overview
This PR adds tab autocompletion support to the Hive CLI using argcomplete, enabling users to autocomplete commands, experiment names, sandbox names, and file paths.
Changes:
- Added argcomplete integration to the CLI argument parser
- Implemented three completer functions (experiment, sandbox, config file) with timeout handling
- Created comprehensive unit tests for all completer functions
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hive_cli/completers.py | New module implementing completer functions for experiments, sandboxes, and config files with timeout and error handling |
| src/hive_cli/main.py | Integrated argcomplete by adding completers to CLI arguments and calling autocomplete() on the parser |
| tests/test_completers.py | Comprehensive unit tests covering success cases, error handling, and filtering for all completer functions |
| pyproject.toml | Added argcomplete dependency |
| README.md | Added documentation for shell completion setup and usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class TimeoutError(Exception): | ||
| """Raised when a completion operation times out.""" | ||
| pass |
There was a problem hiding this comment.
The custom TimeoutError class shadows Python's built-in TimeoutError exception (available since Python 3.3). Rename this to avoid conflicts, for example: CompletionTimeoutError.
| Returns: | ||
| List of experiment names matching the prefix | ||
| """ | ||
| # Set up 2-second timeout |
There was a problem hiding this comment.
The comment states a '2-second timeout' but there's no explanation of why 2 seconds was chosen or how users can adjust this if needed. Consider documenting the rationale for this value.
| Returns: | ||
| List of sandbox pod names matching the prefix | ||
| """ | ||
| # Set up 2-second timeout |
There was a problem hiding this comment.
The comment states a '2-second timeout' but there's no explanation of why 2 seconds was chosen or how users can adjust this if needed. Consider documenting the rationale for this value.
| # Set up 2-second timeout | ||
| signal.signal(signal.SIGALRM, timeout_handler) | ||
| signal.alarm(2) |
There was a problem hiding this comment.
The timeout duration (2 seconds) is hardcoded in both experiment_completer and sandbox_completer. Extract this to a module-level constant like COMPLETION_TIMEOUT_SECONDS = 2 to improve maintainability.
|
/lgtm |
hiverge-robot
added
lgtm
3 participants
No description provided.