-
Notifications
You must be signed in to change notification settings - Fork 0
Migrate CLI from manual argv parsing to tyro with component selection #42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6905d63
7911985
0e6c492
8dbe623
5c11325
ad76b56
79cc843
9bbefdd
c691d30
f45c320
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,24 +1,38 @@ | ||||||||
| import sys | ||||||||
| import asyncio | ||||||||
| import tempfile | ||||||||
| import pathlib | ||||||||
| from typing import Annotated, Optional | ||||||||
| import tyro | ||||||||
| from .orm import initialize_database | ||||||||
| from .ds import main as ds | ||||||||
| from .egg import main as egg | ||||||||
| from .input import main as input | ||||||||
| from .output import main as output | ||||||||
| from .load import main as load | ||||||||
| from .dump import main as dump | ||||||||
|
|
||||||||
| component_map = { | ||||||||
| "ds": ds, | ||||||||
| "egg": egg, | ||||||||
| "input": input, | ||||||||
| "output": output, | ||||||||
| "load": load, | ||||||||
| "dump": dump, | ||||||||
| } | ||||||||
|
|
||||||||
|
|
||||||||
| async def main(addr): | ||||||||
| async def run(addr: str, components: list[str]) -> None: | ||||||||
| engine, session = await initialize_database(addr) | ||||||||
|
|
||||||||
| try: | ||||||||
| try: | ||||||||
| coroutines = [component_map[component](addr, engine, session) for component in components] | ||||||||
| except KeyError as e: | ||||||||
| print(f"error: unsupported component {e}") | ||||||||
|
||||||||
| print(f"error: unsupported component {e}") | |
| component = e.args[0] if e.args else e | |
| print(f"error: unsupported component: {component}") |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value for the component parameter is missing "load" and "dump" components that are defined in component_map (lines 11-21). According to the PR description, it claims to preserve existing behavior, but the original code only ran ds, egg, input, and output. However, since load and dump are now added to the component_map, users might expect them to be available. The inconsistency is that these components exist in the map but aren't mentioned in the help text or included as a clear option.
Consider either:
- Documenting which components are available (e.g., in the help text)
- Validating component names against the
component_mapkeys early with a clear error message listing valid options
| help="Components to run.", | |
| help="Components to run. Available: ds, egg, input, output, load, dump.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
缺少一个 component: list[str] 的功能, 默认是 ["input", "output", "ds", "egg"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 5c11325. Added component: list[str] parameter with default value ["input", "output", "ds", "egg"]. Usage:
# Run all components (default)
ddss
# Run specific components
ddss --component input output
# Run only ds and egg
ddss --component ds egg
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tmpdir variable is created as a TemporaryDirectory object but is never stored or cleaned up properly. This means the temporary directory will be deleted when tmpdir goes out of scope at the end of the main() function, potentially before the async run() function completes and while the database is still in use. This could cause the database file to be deleted while the application is running.
To fix this, the tmpdir should be kept alive for the entire duration of the async operation, or the cleanup should be handled explicitly in the async context.
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The run() function signature in the diff shows parameter types addr: str, components: list[str], but when calling it at line 87, the function is invoked with component (singular) instead of components (plural). However, looking at line 24 in the actual code, the function definition uses components (plural). This appears to be correctly implemented in the actual code, but there's an inconsistency between the parameter name in main() (line 60: component) and in run() (line 24: components).
For consistency and clarity, consider using the same parameter name in both functions, preferably components (plural) since it's a list of multiple components.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ dependencies = [ | |
| "apyds-egg~=0.0.11", | ||
| "prompt-toolkit~=3.0.52", | ||
| "sqlalchemy[aiosqlite,aiomysql,postgresql-asyncpg]~=2.0.45", | ||
| "tyro~=1.0.3", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 记得重新uv lock一下
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in commit 5c11325. Ran |
||
| ] | ||
|
|
||
| [project.urls] | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
run()function is missing type annotations for its parameters. The diff shows type hints in line 24 (async def run(addr: str, components: list[str]) -> None:), but the actual implementation usesasync def run(addr, components)without type annotations. Adding type annotations would improve code clarity and enable better static type checking, which is especially important since the rest of the code is being migrated to use type-annotated interfaces with tyro.