-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,116 @@ | ||||||||||||||||
| """Implementation of the 'default' command to manage default Python version.""" | ||||||||||||||||
|
|
||||||||||||||||
| import json | ||||||||||||||||
| from pathlib import Path as PathlibPath | ||||||||||||||||
|
|
||||||||||||||||
| from .exceptions import ArgumentError, NoInstallsError, NoInstallFoundError | ||||||||||||||||
| from .installs import get_installs, get_matching_install_tags | ||||||||||||||||
|
Comment on lines
+3
to
+7
|
||||||||||||||||
| 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 |
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.
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) |
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 |
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] |
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 |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -117,9 +117,24 @@ def get_installs( | |||||
| except LookupError: | ||||||
| LOGGER.debug("No virtual environment found") | ||||||
|
|
||||||
| # 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() | ||||||
|
Comment on lines
+120
to
+124
|
||||||
| LOGGER.debug("Found saved default install ID: %s", default_id) | ||||||
| for install in installs: | ||||||
| if install["id"] == default_id: | ||||||
| install["default"] = True | ||||||
| LOGGER.debug("Marked %s as default", default_id) | ||||||
| break | ||||||
| except Exception as ex: | ||||||
| LOGGER.debug("Could not load default install marker: %s", ex) | ||||||
|
||||||
| LOGGER.debug("Could not load default install marker: %s", ex) | |
| LOGGER.debug("Could not load default install marker: %s", ex, exc_info=True) |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,59 @@ | ||||
| """Tests for the default command.""" | ||||
|
|
||||
| import pytest | ||||
|
||||
| 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.
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.
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.
Catching and ignoring exceptions here makes the test unable to detect regressions (it will pass even if the command errors for the wrong reason, or if it succeeds unexpectedly). Prefer pytest.raises(...) with a specific expected exception and/or assert on the logged output.
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 test suite doesn’t currently validate the core new behavior (writing <install_dir>/.default and having subsequent resolution respect it). Consider adding an integration-style test that runs py default <TAG>, asserts the marker file contents, and verifies the chosen default via cmd.get_install_to_run('default', ...) or cmd.get_installs().
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_DOCSentry fordefaultwill makehelpoutput list thedefaultcommand twice (once fromPYMANAGER_USAGE_DOCSand again from the per-command loop inBaseCommand.show_usage()that appends allCOMMANDS). Consider removing this tuple fromPYMANAGER_USAGE_DOCSand relying on the standardUSAGE_LINE/HELP_LINEregistration, or updateshow_usage()to de-duplicate entries.