refactor - #621
Open
Akanksha-020 wants to merge 3 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Tool Manager UI to remove duplicated tool label/version/default-version metadata and instead use registry.py as the single source of truth for these values.
Changes:
- Added registry helper accessors (
get_tool_label,get_tool_versions,get_default_version) and updated UI code to use them. - Updated the main Tool Manager UI to derive visible tools, labels, and default install versions from the registry.
- Refactored path handling to use
paths.pyhelpers andpathlib.Pathfor toolManager root/state paths.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/toolManager/updater_gui.py | Replaced hardcoded available-version lists with registry-derived versions for the updater UI. |
| src/toolManager/tool_manager_windows.py | Minor refactor to avoid duplicated MSYS2_PATH assignment. |
| src/toolManager/registry.py | Added helper functions to centralize label/version/default-version lookups. |
| src/toolManager/main.py | Removed hardcoded tool label/version maps; now pulls labels/versions/defaults and tool lists from registry and uses paths helpers. |
| src/toolManager/gui_fixed.py | Refactored backend path construction to use paths.py and Path instead of os.path. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/toolManager/updater_gui.py:109
- The registry version lists currently don’t match what the updater scripts accept. For example,
update-kicad-final.shonly supports 6.0.11/7.0.11/8.0.9, but the registry’skicadversions arelatest, 9, 8, 7, 6; selectinglatestor9here will cause the script to exit with “Invalid KiCad version specified”. Similar mismatches exist for GHDL and Verilator script-supported versions. Either make registry versions platform/script-aware or update the scripts to accept the registry’s versions before wiring the GUI toget_tool_versions().
for tool_id in ('kicad', 'ngspice', 'ghdl', 'verilator'):
metadata = get_tool_metadata(tool_id)
if metadata:
self.available_versions[metadata.label] = get_tool_versions(tool_id)
src/toolManager/updater_gui.py:109
self.available_versionsis populated only whenget_tool_metadata()returns a spec. The UI later indexesself.available_versions[package_name]for all four packages, so any missing metadata will raise aKeyErrorduring UI construction. Initialize defaults for the expected labels (or use.get(..., [])) so the window can still render even if registry data is missing/incomplete.
self.available_versions = {}
for tool_id in ('kicad', 'ngspice', 'ghdl', 'verilator'):
metadata = get_tool_metadata(tool_id)
if metadata:
self.available_versions[metadata.label] = get_tool_versions(tool_id)
src/toolManager/gui_fixed.py:23
- This file still contains a hardcoded
TOOLSdictionary with per-tool version lists (and other tool metadata) even though the PR description says the registry should be the single source of truth for tool names/versions/defaults. Consider migratinggui_fixed.pyto read tool metadata fromregistry.py(or explicitly documenting why this UI remains separate), otherwise version updates must be duplicated in multiple places.
from constants import IS_WINDOWS, IS_LINUX
from paths import get_toolmanager_root
BASE_DIR = get_toolmanager_root()
src/toolManager/gui_fixed.py:21
Pathis imported but never used after switchingBASE_DIRtoget_toolmanager_root(). Removing the unused import avoids lint noise and keeps the module header minimal.
PYTHON = sys.executable
from pathlib import Path
from constants import IS_WINDOWS, IS_LINUX
from paths import get_toolmanager_root
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.
Related Issues
Purpose
Remove duplicated tool label and version metadata from the Tool Manager UI code and make the existing registry the single source of truth for tool names, supported versions, and default install versions
Approach
[registry.py] now exposes small helper functions for label, version, and default-version lookup. [main.py] no longer keeps separate hardcoded label/version maps; it reads tool names and install defaults from the registry, and the About tab now renders supported versions directly from registry data. [updater_gui.py] now builds its available-version list from the registry instead of maintaining a second hardcoded copy, while keeping the existing script mapping unchanged.