Conversation
Moved argument parsing from __main__.py to suite/main.py to create a single source of truth for CLI interfaces. The duct command now delegates to 'con-duct run', making __main__.py a pure execution engine with minimal dependencies. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Rename Arguments to RunArguments for clarity - Extract create_parser() method as single source of truth for CLI arguments - Use parent parser approach for run subcommand to share argument definitions - Set add_help=False on run subparser to avoid -h/--help conflict with parent - Update run_command() to construct RunArguments from parsed Namespace - Remove fixed issue from TODO.md Now both `con-duct run --help` and `duct --help` properly display all available options without conflicts. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove main() and __name__ == "__main__" block from __main__.py - Add duct_main() to suite/main.py that delegates to con-duct run - Update setup.cfg entry point: duct → con_duct.suite.main:duct_main - __main__.py is now a pure execution engine with no CLI knowledge This keeps __main__.py standalone and dependency-free, while all CLI entry points and delegation logic live in suite/main.py where they belong. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Rename __main__.py → duct_main.py (no longer needs special name)
- Move suite/main.py → cli.py (all CLI logic)
- Move suite/{ls,plot,pprint_json}.py → con_duct/ (flatten structure)
- Update all imports: __main__ → duct_main, suite.* → con_duct.*
- Update entry points in setup.cfg to point to cli module
- Remove now-empty suite/ directory
Simpler, flatter structure with clearer naming:
- duct_main.py: Pure execution engine
- cli.py: All CLI logic and entry points
- Other files directly in con_duct/
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Avoid confusion between duct_main() function and duct_main.py module. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🧪 CI InsightsHere's what we observed from your CI run for e4631fc. 🟢 All jobs passed!But CI Insights is watching 👀 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #327 +/- ##
==========================================
- Coverage 91.34% 90.79% -0.55%
==========================================
Files 7 7
Lines 947 945 -2
Branches 125 125
==========================================
- Hits 865 858 -7
- Misses 59 66 +7
+ Partials 23 21 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Python 3.14 changed subparser behavior to no longer inherit parent usage strings in prog values. This caused inconsistent help output and test failures. Set explicit prog parameter on all subparsers to ensure consistent, clear usage strings across Python versions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
=== Do not change lines below ===
{
"chain": [],
"cmd": "./.update-readme-help.py",
"exit": 0,
"extra_inputs": [],
"inputs": [],
"outputs": [],
"pwd": "."
}
^^^ Do not change lines above ^^^
Remove circular dependency between duct_main and cli modules by refactoring execute() to accept individual parameters instead of RunArguments dataclass. This makes the execution logic completely independent of CLI parsing, improving modularity and preparing for config file support. Changes: - execute() now accepts 14 individual parameters instead of RunArguments - Remove TYPE_CHECKING import of RunArguments from duct_main - Simplify run_command() to pass args directly without reconstruction - Add RunArguments.execute() convenience method for tests and library usage - Update test imports to use RunArguments directly (not aliased) - Update test calls to use args.execute() pattern 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
=== Do not change lines below ===
{
"chain": [],
"cmd": "./.update-readme-help.py",
"exit": 0,
"extra_inputs": [],
"inputs": [],
"outputs": [],
"pwd": "."
}
^^^ Do not change lines above ^^^
|
pypy310 failure is unrelated, will be fixed by #331 |
yarikoptic
left a comment
There was a problem hiding this comment.
I think it is worth simplifying while unifying so all commands do have similar pattern in defining/checking/executing.
There was a problem hiding this comment.
Pull request overview
This PR successfully consolidates two separate CLI tools (duct and con-duct) into a unified CLI structure where duct becomes a convenience alias for con-duct run. The refactoring significantly improves maintainability by centralizing argument parsing and CLI logic while maintaining backward compatibility.
Key changes:
- Unified CLI with
con-ductas the main entry point offering subcommands:run,pp,plot, andls ductcommand now delegates tocon-duct runviaos.execvpfor transparent process replacement- Refactored
execute()function to accept explicit parameters instead of an Arguments dataclass, improving flexibility and testability
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/con_duct/cli.py | New unified CLI module combining argument parsing for all subcommands with shared logging configuration |
| src/con_duct/duct_main.py | Refactored to remove CLI-specific code (Arguments dataclass, main function), keeping only execution logic |
| src/con_duct/plot.py | Newly consolidated plot functionality (moved from suite/plot.py) |
| src/con_duct/ls.py | Updated imports to reference duct_main instead of main |
| src/con_duct/pprint_json.py | Updated imports to reference duct_main instead of main |
| setup.cfg | Updated entry points: duct → cli:duct_entrypoint, con-duct → cli:main |
| test/utils.py | Added run_duct_command() helper for cleaner test execution without argument parsing |
| test/test_validation.py | Updated to use run_duct_command() helper and expect ValueError instead of ArgumentError |
| test/test_execution.py | Removed test_outputs_none_quiet (replaced by E2E test), updated all tests to use run_duct_command() |
| test/test_e2e.py | Added test_logging_levels() for E2E validation of --quiet and --log-level NONE behavior |
| test/test_arg_parsing.py | Updated to import _create_run_parser from cli module and test parser directly |
| test/test_suite.py | Updated all imports from suite.main to cli module |
| test/test_*.py (multiple) | Updated imports from con_duct.main to con_duct.duct_main throughout test suite |
| README.md | Restructured documentation to reflect unified CLI structure with subcommands |
| CLAUDE.md | Updated to reflect new project structure with single entry point |
| .gitignore | Added AI tool directories (.serena/, .local*) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@CodyCBakerPhD, yarik has had trouble finding time to review this, would you mind having a first pass? |
| duct = con_duct.cli:duct_entrypoint | ||
| con-duct = con_duct.cli:main |
There was a problem hiding this comment.
I see you are still keeping both defined and ever so slightly different here
Is there a plan to deprecate one to avoid confusion of which to use?
There was a problem hiding this comment.
I see - from https://github.com/con/duct/pull/327/files duct is merely an alias
There was a problem hiding this comment.
no plan to deprecate, duct is now just be a shortcut for con-duct run, and they should behave exactly the same.
There was a problem hiding this comment.
OK - towards the eternal spirit of PEP 20 (Zen of Python - line 12 - "There should be one-- and preferably only one --obvious way to do it.") perhaps put some thought into going one way or another to remove confusion, reduce number of things to maintain, and remove a source of possible bugs (if wires get crossed or imports/invocations get changed)
There was a problem hiding this comment.
Its so thin I hope it will be fine. I prefer duct actually, its so simple-- but having it be its own separate CLI isn't worth it. (adding config to both CLIs, or shared flags like --log-level for example). IMO if there is a maintenance burden I would deprecate duct in favor of con-duct run, but i'd prefer to keep both if its not a problem.
+900/-800 lines changed always scary 😆what if 1 important line missing @asmacdo Aside from trusting tests, how best do you recommend testing this PR works? Ensuring the rebased config/other PRs work well? Trying a couple commands locally? Also what is your opinion on these codecov CI checks |
Fair enough, I just wanted to make it clear thats because it was moved
I think the best way is trying some commands locally and making sure the flags work (both duct and con-duct run), and especially having a look at the --help output. ( The config PR seems to have survived the rebase unscathed (it was already based on this branch). Since those tests pass, I think we are ok (since its been a while I plan to do a careful self-review of that before taking out of draft state)
Coverage seems fine IMO-- we don't have tests for CLI parsing, since thats a big chunk of what got moved, the patch% is pretty low. I dont love that its not covered, but testing argparse usage doesnt add much value IMO (interested to hear if you disagree) |
I meant do you find it useful that they show as failing on the PR checks |
The red x is a little annoying, but it has been useful as a reminder to check what was missed. I don't feel strongly either way though. |
|
Not familiar enough with the logging level feature to thoroughly interrogate that, but everything LGTM and seemed to work at a high level (minus a small likely completely separate issue I will file) Since this is at last becoming more of a true 'Python package' and less of a single-script one-off, do you have any intention of eventually controlling proper scopes and exposure per module? (so I can't do something misleading like |
|
Currently its still primarily intended to be used as a CLI, though library usage would be very interesting! So far I haven't got any real-world need for it-- unless youve got one? (IMO we should wait for a need before investing much). In the meantime though, I think it would be good to proceed with that in mind so we dont make it harder to do. If you would like to use it as a library, I would love to discuss on a new issue! (One case that comes to mind would be a datalad plugin |
I would love to create a benchmarking library that leverages this :) (mentally reserving Our work on NWB Benchmarks relied on hacking a version of Airspeed Velocity, which set out to do way too much as a package that essentially wrapped |
| ## Build & Test Commands | ||
| - Use `python -m venv .venv` to create a virtual environment | ||
| - Use `source .venv/bin/activate` to activate a virtual environment | ||
| - A virtual environment should be used (see .local.CLAUDE.md or ~/.claude/CLAUDE.md if they exist for venv locations) |
There was a problem hiding this comment.
Below you use @ for referencing files but not here
yarikoptic
left a comment
There was a problem hiding this comment.
Left one non consequential comment, let's see it on action
This PR combines 2 CLIs into a single CLI for simplicity, maintainability, and (as we learned when adding config) extensibility.
Prior to this PR the project was separated into 2 CLIs, maintained separately,
ductandcon-duct. This worked fine, but IMO was always a little awkward. However problems and complexity have crept up:This PR sets us up to:
Note to the reviewer: 26 files changed is not scary! Most files are just changing imports.
Breaking Change:
duct_main.py cannot be directly executed anymore. If users wish to use
ductwithout installingcon-duct, they can still do so by fetchingduct_main.pyand executing their own python usingduct_main.execute().TODO: