-
Notifications
You must be signed in to change notification settings - Fork 5
Refactor: Break up duct_main.py into smaller modules #385
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
Conversation
Move ENV_PREFIXES and SUFFIXES to dedicated _constants module. Remove unused DEFAULT_LOG_LEVEL. Part of #372 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
refactor: extract constants to _constants.py
Move enums (Outputs, RecordTypes, SessionMode) and dataclasses (SystemInfo, ProcessStats, LogPaths, Averages, Sample) to _models.py. Create _utils.py with assert_num helper. Part of #372 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move parse_version from utils.py to _utils.py and remove utils.py. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
refactor: extract models and enums to _models.py
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
refactor: extract SummaryFormatter to _formatter.py
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
refactor: extract SigIntHandler to _signals.py
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
refactor: extract sampling functions to _sampling.py
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
refactor: extract IO utilities to _output.py
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
refactor: extract Report and monitor_process to _tracker.py
This file is no longer directly executable - it's a module imported by the CLI. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #385 +/- ##
==========================================
+ Coverage 91.38% 91.81% +0.43%
==========================================
Files 8 15 +7
Lines 1056 1112 +56
Branches 138 138
==========================================
+ Hits 965 1021 +56
Misses 69 69
Partials 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull request overview
This PR successfully refactors duct_main.py by extracting code into smaller, focused modules as outlined in issue #372. The refactoring reduces duct_main.py from approximately 600 lines to 180 lines, improving maintainability and code organization.
Changes:
- Extracted 8 new private modules:
_constants.py,_models.py,_utils.py,_formatter.py,_signals.py,_sampling.py,_output.py, and_tracker.py - Updated imports across all source and test files to reference the new module structure
- Maintained
DUCT_OUTPUT_PREFIXandEXECUTION_SUMMARY_FORMATinduct_main.pyas noted in the deferred work section
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/con_duct/_constants.py | New module containing __schema_version__, ENV_PREFIXES, and SUFFIXES constants |
| src/con_duct/_models.py | New module with data models (Enums: Outputs, RecordTypes, SessionMode; Dataclasses: SystemInfo, ProcessStats, LogPaths, Averages, Sample) |
| src/con_duct/_utils.py | New module with utility functions (assert_num, parse_version) |
| src/con_duct/_formatter.py | New module containing SummaryFormatter class for output formatting |
| src/con_duct/_signals.py | New module with SigIntHandler for signal handling |
| src/con_duct/_sampling.py | New module with platform-specific process sampling functions |
| src/con_duct/_output.py | New module with TailPipe class and I/O utility functions |
| src/con_duct/_tracker.py | New module containing Report class, monitor_process function, and __version__ |
| src/con_duct/duct_main.py | Reduced to orchestration function execute() and two constants, imports from new modules |
| src/con_duct/cli.py | Updated imports to reference new module locations |
| src/con_duct/ls.py | Updated imports to reference new module locations |
| src/con_duct/json_utils.py | Updated imports to reference _constants module |
| src/con_duct/pprint_json.py | Updated imports to reference _formatter module |
| test/*.py | Updated all test imports to reference new module locations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for f in file_list: | ||
| try: | ||
| f.close() | ||
| except Exception: |
Copilot
AI
Jan 24, 2026
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.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| except Exception: | |
| # Best-effort close: ignore errors (e.g., already closed or invalid objects). |
| from pathlib import Path | ||
| from utils import run_duct_command | ||
| from con_duct.duct_main import SUFFIXES | ||
| from con_duct._constants import SUFFIXES |
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.
Well, this is the sort of thing that is not great (but we can fix in follow-up focusing on import control)
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.
Yep I imagine constants should be public, but I figured for this approach, lets start off a bit ham-handed, and make everything private so we can be minimal as we transition to library usage.
|
|
||
| import pytest | ||
| from con_duct.duct_main import assert_num | ||
| from con_duct._utils import assert_num |
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.
Testing helpers generally fall under a .testing public module
Can fix in follow up
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.
this is used outside of tests also, but im open to move later.
| from con_duct import duct_main | ||
| from con_duct.duct_main import SUFFIXES, Outputs | ||
| from con_duct._constants import SUFFIXES | ||
| from con_duct._models import Outputs |
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.
Basically, anything you import from the main package like this is a good example of something that should be publicly exposed either at root level or via some public submodule
Tests like this help indicate what might be intended to be exposed
|
Overall LGTM - some discussion points for the importing specific followup above @asmacdo I assume you have tried out this branch in practice (not just tests) and found it working to your liking? Left my approval FWIW (have read-only permissions so doesn't mean much here) |
|
Keep in mind one of the great benefits of privatizing this kind of stuff (after fixing import structure in follow up) is after that you can feel free to alter the file/directory structure freely at will without worrying about breaking anything |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
I have tested locally, and it works. This isn't perfect, but I think it provides a nice structure to build on, so I'll merge after CI comes back green. |
Summary
Completes the module extraction from
duct_main.pyas outlined in #372.Extracted modules:
_formatter.py- SummaryFormatter class_signals.py- SigIntHandler_sampling.py- Platform-specific process sampling_output.py- TailPipe, output preparation, file cleanup_tracker.py- Report class and monitor_process functionResult:
duct_main.pyreduced from ~600 lines to ~180 linesexecute()orchestration function and two constantsDeferred:
DUCT_OUTPUT_PREFIXandEXECUTION_SUMMARY_FORMATtocli.py(requires refactoringls.pyparameter handling - tracked in Move CLI defaults to cli.py and refactor ls.py parameter handling #386)cli.py→_cli.pyrename: This would have added a lot more, and might be better to just move tocli/Closes Refactor duct_main.py into smaller modules #372
🤖 Generated with Claude Code