-
Notifications
You must be signed in to change notification settings - Fork 36
Change default version command - Enhancement #263 #264
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
base: main
Are you sure you want to change the base?
Conversation
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
Adds a new py default subcommand to show or set the default Python runtime, persisting the selection via a .default marker file and reflecting it in install discovery.
Changes:
- Introduces
DefaultCommandand its implementation indefault_command.py - Updates install discovery to read
.defaultand mark the matching install as default - Adds initial tests for the new command and ignores
/.venv/
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_default_command.py | Adds tests targeting DefaultCommand behavior (currently light on assertions). |
| src/manage/installs.py | Reads <install_dir>/.default and marks the matching install as default=True. |
| src/manage/default_command.py | Implements the default command logic (show current / set new default). |
| src/manage/commands.py | Registers the default command, CLI schema hooks, and usage/help text updates. |
| .gitignore | Ignores a root /.venv/ directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (f"{EXE_NAME} default !B!<TAG>!W!\n", | ||
| "Set the default Python version to use when no specific version is " + | ||
| "requested. Use without !B!<TAG>!W! to show the current default."), |
Copilot
AI
Jan 31, 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.
The new PYMANAGER_USAGE_DOCS entry for default will make help output list the default command twice (once from PYMANAGER_USAGE_DOCS and again from the per-command loop in BaseCommand.show_usage() that appends all COMMANDS). Consider removing this tuple from PYMANAGER_USAGE_DOCS and relying on the standard USAGE_LINE/HELP_LINE registration, or update show_usage() to de-duplicate entries.
| (f"{EXE_NAME} default !B!<TAG>!W!\n", | |
| "Set the default Python version to use when no specific version is " + | |
| "requested. Use without !B!<TAG>!W! to show the current default."), |
| def _load_default_install_id(install_dir): | ||
| """Load the saved default install ID from the marker file.""" | ||
| try: | ||
| default_file = _get_default_config_file(install_dir) | ||
| if default_file.exists(): |
Copilot
AI
Jan 31, 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.
_load_default_install_id() is currently unused. Either remove it, or use it in _show_current_default() so the command can report the saved default ID even when it isn’t present in the current installs list.
| default_file = _get_default_config_file(install_dir) | ||
| default_file.parent.mkdir(parents=True, exist_ok=True) | ||
| default_file.write_text(install_id, encoding="utf-8") | ||
| LOGGER.info("Default Python version set to: !G!%s!W!", install_id) |
Copilot
AI
Jan 31, 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.
This logs a “Default … set” message, and _set_default_version() logs another one after calling _save_default_install_id(). Consider keeping a single user-facing log line to avoid duplicate console output.
| LOGGER.info("Default Python version set to: !G!%s!W!", install_id) | |
| LOGGER.debug("Saved default install ID: %s", install_id) |
| # Check for a saved default install marker | ||
| try: | ||
| default_file = Path(install_dir) / ".default" | ||
| if default_file.exists(): | ||
| default_id = default_file.read_text(encoding="utf-8").strip() |
Copilot
AI
Jan 31, 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.
The new .default marker behavior introduced here isn’t covered by tests. Please add a test (e.g., using patched_installs + tmp_path) that writes an install ID into <install_dir>/.default and asserts get_installs() marks the matching install with default=True (and that it affects get_install_to_run(..., tag='default') as intended).
| if not matching: | ||
| raise NoInstallFoundError(tag=tag) | ||
|
|
||
| selected_install, selected_run_for = matching[0] |
Copilot
AI
Jan 31, 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.
selected_run_for is assigned but never used. Rename it to _ (or otherwise use it) to avoid unused-variable warnings and make intent clearer.
| selected_install, selected_run_for = matching[0] | |
| selected_install = matching[0][0] |
| except NoInstallsError: | ||
| LOGGER.info("No Python installations found.") | ||
| return | ||
|
|
Copilot
AI
Jan 31, 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.
_show_current_default() expects cmd.get_installs() to raise NoInstallsError, but BaseCommand.get_installs() returns an empty list when there are no installs. As written, the no-installs case will print “No explicit default set” instead of “No Python installations found.” Handle if not installs: explicitly (and consider removing the unreachable except NoInstallsError:).
| if not installs: | |
| LOGGER.info("No Python installations found.") | |
| return |
| else: | ||
| # Set new default | ||
| tag = " ".join(cmd.args[0:1]) # Take the first argument as the tag | ||
| _set_default_version(cmd, tag) |
Copilot
AI
Jan 31, 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.
_set_default_version() may raise NoInstallFoundError, but execute() doesn’t handle it. Since manage.main() treats uncaught exceptions as INTERNAL ERROR, py default <TAG> on an unknown tag will look like a crash. Catch NoInstallFoundError here (log a user-facing message and raise SystemExit(1), or convert to ArgumentError) to match other commands’ error-handling patterns.
| _set_default_version(cmd, tag) | |
| try: | |
| _set_default_version(cmd, tag) | |
| except NoInstallFoundError: | |
| LOGGER.error("No Python installation found matching tag '%s'.", tag) | |
| raise ArgumentError(f"No Python installation found matching tag '{tag}'.") from None |
| import json | ||
| from pathlib import Path as PathlibPath | ||
|
|
||
| from .exceptions import ArgumentError, NoInstallsError, NoInstallFoundError | ||
| from .installs import get_installs, get_matching_install_tags |
Copilot
AI
Jan 31, 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.
Unused imports: json, PathlibPath, and get_installs are not used in this module. Please remove them to keep the module minimal and avoid lint/static-analysis warnings.
| import json | |
| from pathlib import Path as PathlibPath | |
| from .exceptions import ArgumentError, NoInstallsError, NoInstallFoundError | |
| from .installs import get_installs, get_matching_install_tags | |
| from .exceptions import ArgumentError, NoInstallsError, NoInstallFoundError | |
| from .installs import get_matching_install_tags |
| def test_default_command_no_args_no_installs(assert_log): | ||
| """Test default command with no arguments and no installations.""" | ||
| cmd = commands.DefaultCommand([commands.DefaultCommand.CMD], None) | ||
| # This should handle the case gracefully | ||
| # We expect it to either show a message about no installs or show current default |
Copilot
AI
Jan 31, 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.
This test is non-deterministic and doesn’t assert behavior. DefaultCommand(..., root=None) may read installs from the real environment (sys.prefix/pkgs), and the test passes even if execute() produces incorrect output. Use tmp_path/monkeypatch (e.g., patched_installs) and assert on output/state so the test validates the no-installs behavior.
| @@ -0,0 +1,59 @@ | |||
| """Tests for the default command.""" | |||
|
|
|||
| import pytest | |||
Copilot
AI
Jan 31, 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.
Import of 'pytest' is not used.
| import pytest |
No description provided.