Repository navigation
refactor(cli): centralize catalog default - #37
Conversation
|
👋 Hi @bryanwhl — thank you so much for your first contribution to HFlow! A maintainer will review your pull request as soon as possible. In the meantime:
💡 Tip: one open pull request per contributor at a time. Issues with an assignee are taken; everything else is fair game. We are excited to have you here and appreciate your help making the project better! 🙌 |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
kstonekuan
left a comment
There was a problem hiding this comment.
Thanks @bryanwhl, and welcome! Validated locally: full quality gate and test suite pass (293 tests), CI green on both Pythons. Special credit for the string-vs-Path reasoning in the description: you are right that Path("./data/catalog") would normalize away the ./ in the help text and make the argument's type inconsistent between defaulted and user-supplied runs. That is exactly the kind of trade-off note that makes a review fast. The honest report of the macOS-only test failures (/usr/bin/ffmpeg assumption, missing drawtext filter in Homebrew ffmpeg) is appreciated too; those are pre-existing platform issues, not caused by this change, and worth their own issue. Merging.
Summary
Centralize the catalog CLI default in
DEFAULT_CATALOG_DIRand reuse it for both thecurateandstaledefaults and help text.Why
The catalog path was repeated four times, so future changes could leave command defaults and help text out of sync. The constant remains the string
./data/catalograther than aPathexpression so this refactor preserves both argparse's default value type and the exact user-visible help text.Closes #31
Validation
uv run pytest tests/test_catalog_curation.py -q— 16 passeduv run ruff check --fix— passeduv run ruff format— 84 files unchangeduv run ty check— passeduv run pytest -q— 285 passed, 6 skipped, 5 unrelated local-platform failures: one test assumes/usr/bin/ffmpeg, and four require an ffmpeg build with thedrawtextfilter (the installed Homebrew ffmpeg lacks it)Checklist
uv run ruff check --fix,uv run ruff format, anduv run ty check.