Skip to content
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

deprecate "installer.modern-installation false" #8988

Merged

Conversation

dimbleby
Copy link
Contributor

As proposed at #8987

I started with warnings.warn but something about all the cleo io cleverness results in those messages disappearing. Anyway this deprecation is more user-facing than developer-facing, so regular warning output is probably fine.

I'd be happy to see this deprecation in the upcoming release, might as well get it out there and see who squeals. But no big deal if it misses the cut.

@dimbleby dimbleby force-pushed the deprecate-non-modern-installation branch from 0bcbcd9 to 3d794ce Compare February 18, 2024 18:27
@dimbleby dimbleby force-pushed the deprecate-non-modern-installation branch from 3d794ce to 1deba97 Compare February 18, 2024 18:33
radoering
radoering previously approved these changes Feb 19, 2024
@@ -68,6 +68,17 @@ def __init__(
self._use_modern_installation = config.get(
"installer.modern-installation", True
)
if not self._use_modern_installation:
self._io.write_line(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing: Shouldn't we use write_error_line() for warnings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some do, some don't: if there is a rule then it is not consistently followed. The stdout from these commands is not really suitable for parsing anyway so I don't suppose it much matters.

I'm not at the right computer to do anything about this now, feel free do any of tweak it yourself / ask me to do it later / not bother.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are probably right that it is inconsistent and does not matter.

Interestingly enough, I see random test failures on Windows if I write the warning to error out. It's always a PermissionError in

shutil.rmtree(venv.path)

but in different tests, for example one or two of

tests/console/commands/self/test_add_plugins.py::test_add_with_constraint
tests/console/commands/self/test_add_plugins.py::test_add_with_git_constraint_with_extras
tests/console/commands/self/test_remove_plugins.py::test_remove_installed_package
tests/console/commands/self/test_remove_plugins.py::test_remove_installed_package_dry_run
tests/installation/test_executor.py::test_executor_should_write_pep610_url_references_for_editable_git

Maybe, a reason to just keep it as is, even if this is a bit unsettling...

@radoering radoering merged commit b49d2dd into python-poetry:master Feb 20, 2024
20 checks passed
@dimbleby dimbleby deleted the deprecate-non-modern-installation branch February 20, 2024 15:56
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants