Transitional PR for typer CLI #507
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR transitions the CLI infrastructure from argparse to typer, consolidating CLI code and removing redundant version handling.
- Migrates eido module from argparse to typer-based CLI
- Consolidates version handling at the main package level
- Removes redundant pephubclient version callback (now handled centrally)
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 |
|---|---|
| peppy/pephubclient/cli.py | Removed duplicate version callback and unused imports, relying on parent app for version display |
| peppy/pephubclient/init.py | Cleaned up module by removing version constants and commented code, keeping only author attribution |
| peppy/eido/cli.py | New typer-based CLI implementation migrating functionality from argparser.py |
| peppy/eido/argparser.py | Removed as functionality migrated to cli.py |
| peppy/cli.py | Refactored from argparse to typer, integrating eido and phc subcommands with centralized version handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger_level = _configure_logging(verbosity, logging_level, dbg) | ||
| logger_kwargs = {"level": logger_level, "devmode": dbg} | ||
|
|
||
| global _LOGGER |
There was a problem hiding this comment.
Using a global variable for the logger is an anti-pattern that can cause issues in testing and concurrent execution. Consider passing the logger through the context object (ctx.obj) or using the logger instance returned by init_logger directly within each command function.
| sys.exit(1) | ||
|
|
||
| if paths_: | ||
| paths = {y[0]: y[1] for y in [x.split("=") for x in paths_]} |
There was a problem hiding this comment.
This code will raise an IndexError if any path value doesn't contain an '=' character. The split may return a list with fewer than 2 elements. Add validation or use split with maxsplit parameter and verify the result length.
| paths = {y[0]: y[1] for y in [x.split("=") for x in paths_]} | |
| paths = {} | |
| for x in paths_: | |
| parts = x.split("=", 1) | |
| if len(parts) != 2: | |
| raise EidoFilterError( | |
| f"Invalid path argument '{x}'. Expected format: key=value" | |
| ) | |
| paths[parts[0]] = parts[1] |
| if sample_name: | ||
| try: | ||
| sample_name = int(sample_name) | ||
| except ValueError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except ValueError: | |
| except ValueError: | |
| # If sample_name is not an integer, leave it as-is (likely a string identifier) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.