fix: Refactor command and model modules for readability and maintainability (#231)#232
Open
sergio-sisternes-epam wants to merge 10 commits intomicrosoft:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the phased refactor of APM command/model code by extracting shared constants and typed result containers, reorganizing dependency/uninstall/compile/deps modules into smaller submodules, and tightening some overly-broad exception handling.
Changes:
- Introduces
src/apm_cli/constants.pyand replaces hard-coded filenames/dirs across commands/models. - Extracts typed result containers (
InstallResult,PrimitiveCounts) and updates install tests/mocks accordingly. - Splits large modules into packages/submodules (dependency models, uninstall command, deps helpers, compile watcher) and narrows some
except Exceptionclauses.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_install_command.py | Updates mocks to use InstallResult instead of a tuple. |
| src/apm_cli/models/validation.py | Uses shared constants for filenames/dirs; narrows exception types. |
| src/apm_cli/models/results.py | Adds typed dataclass result containers. |
| src/apm_cli/models/dependency/types.py | Extracts enums/dataclasses and parse_git_reference. |
| src/apm_cli/models/dependency/reference.py | Moves DependencyReference parsing logic into a dedicated module. |
| src/apm_cli/models/dependency/mcp.py | Moves MCPDependency into its own module. |
| src/apm_cli/models/dependency/init.py | Re-exports dependency types/models from the new package. |
| src/apm_cli/models/dependency.py | Removes the former 1000+ line dependency “god file”. |
| src/apm_cli/models/init.py | Exposes InstallResult/PrimitiveCounts at the models package level. |
| src/apm_cli/constants.py | Adds shared constants (filenames/dirs) and InstallMode enum. |
| src/apm_cli/commands/update.py | Narrows exception handling in update flow. |
| src/apm_cli/commands/uninstall/engine.py | Introduces uninstall engine helpers (validation/removal/cleanup/reintegration). |
| src/apm_cli/commands/uninstall/cli.py | Introduces a new uninstall CLI wiring that delegates to the engine. |
| src/apm_cli/commands/uninstall/init.py | Exposes uninstall command and helper functions from the new package. |
| src/apm_cli/commands/uninstall.py | Removes the former monolithic uninstall implementation. |
| src/apm_cli/commands/runtime.py | Narrows broad exception handling. |
| src/apm_cli/commands/run.py | Narrows broad exception handling in script listing/execution. |
| src/apm_cli/commands/prune.py | Uses shared constants; narrows exceptions; restructures deployed-file cleanup loop. |
| src/apm_cli/commands/mcp.py | Narrows broad exception handling around registry calls. |
| src/apm_cli/commands/list_cmd.py | Narrows broad exception handling in Rich fallback. |
| src/apm_cli/commands/install.py | Uses shared constants; adds InstallMode; returns InstallResult; extracts integration helper. |
| src/apm_cli/commands/init.py | Uses shared constants; narrows prompt exception handling. |
| src/apm_cli/commands/deps/cli.py | Fixes imports, uses constants, and moves helper logic to deps/_utils.py. |
| src/apm_cli/commands/deps/_utils.py | New helper module for deps commands (counts, nested detection, updates). |
| src/apm_cli/commands/deps/init.py | Exposes deps CLI and helpers via package exports. |
| src/apm_cli/commands/config.py | Uses shared constants for apm.yml checks. |
| src/apm_cli/commands/compile/watcher.py | Extracts watch-mode logic into a dedicated module. |
| src/apm_cli/commands/compile/cli.py | Uses shared constants; factors out summary/next-step display; imports watcher module. |
| src/apm_cli/commands/compile/init.py | Exposes compile command and helper functions via package exports. |
| src/apm_cli/commands/_helpers.py | Uses shared constants; narrows exceptions; updates gitignore handling. |
| .vscode/tasks.json | Adds local VS Code tasks (currently machine-path specific). |
You can also share your feedback on Copilot code review. Take the survey.
…icrosoft#231) - Add src/apm_cli/constants.py with shared file/directory name constants - Replace magic strings (apm.yml, apm.lock, apm_modules, etc.) with constants - Narrow 42 broad 'except Exception' handlers to specific types (OSError, ValueError, yaml.YAMLError, KeyError, etc.) - Scope: commands/ and models/validation.py
…osoft#231) - install.py: Extract _integrate_package_artifacts() to deduplicate ~170-line cached/fresh integration loops. Add IntegrationResult namedtuple. Define _resolve_and_filter_deps, _setup_integrators, _parallel_download_packages, _finalize_install helper stubs. - uninstall.py: Break 560-line uninstall() into 7 focused helpers: _validate_uninstall_packages, _dry_run_uninstall, _remove_packages_from_disk, _cleanup_transitive_orphans, _sync_integrations_after_uninstall, _cleanup_stale_mcp. - dependency.py: Break 430-line DependencyReference.parse() into _detect_virtual_package, _parse_ssh_url, _parse_standard_url. All 1693 tests pass. No behavioral changes.
…osoft#231) - Add InstallMode enum (ALL, APM, MCP) in constants.py - Replace --only string dispatch with enum comparison in install.py - Add VirtualPackageType enum (FILE, COLLECTION, SUBDIRECTORY) in dependency.py - Add virtual_type property; refactor is_virtual_* to delegate to it - Skip TargetType Literal→Enum: too many string comparisons across 6+ files All 1693 tests pass. No behavioral changes.
…t#231) Convert 4 files into packages with __init__.py re-exports: - commands/deps.py (880→3 files): cli.py (597), _utils.py (299) - commands/compile.py (740→3 files): cli.py (580), watcher.py (172) - commands/uninstall.py (523→3 files): cli.py (193), engine.py (357) - models/dependency.py (974→4 files): reference.py (787), mcp.py (136), types.py (61) All import paths preserved via __init__.py re-exports. install.py kept as single file (test patch compatibility). All 1693 tests pass. No behavioral changes.
…icrosoft#231) - Add InstallResult and PrimitiveCounts dataclasses in models/results.py - Wire InstallResult into _install_apm_dependencies() replacing tuple return - Apply guard clauses to reduce nesting in install.py (9→8), compile/cli.py (10→8), prune.py (8→7) - Extract _display_single_file_summary() and _display_next_steps() from compile/cli.py - Skip Rich/text rendering strategy pattern (no clear duplication) All 1693 tests pass. No behavioral changes.
Reverts all 42 Phase 1 exception handler narrowings back to `except Exception` to preserve defensive error handling in the CLI. The original broad handlers were intentional — they log errors and continue gracefully, preventing the tool from crashing on unexpected errors. Constants extraction and all other refactoring changes from Phases 1–5 are preserved.
- Remove 4 unused helper functions from install.py (_resolve_and_filter_deps, _setup_integrators, _parallel_download_packages, _finalize_install) - Wrap long import line in install.py to respect Black line-length limit - Replace silent 'except Exception: pass' with _rich_warning() calls in uninstall/cli.py (lockfile update) and uninstall/engine.py (re-integration)
sergio-sisternes-epam
force-pushed
the
fix/231-refactor-command-model-modules
branch
from
f0285cd to
425f12e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Phased refactoring of command and model modules to bring all files under code quality thresholds (400-line file limit, 30-statement function limit).
Fixes #231
Type of change
Testing