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

Replace poetry shell command with poetry env activate #9763

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Secrus
Copy link
Member

@Secrus Secrus commented Oct 12, 2024

Pull Request Check List

Resolves: personal grudge towards poetry shell

  • Added tests for changed code.
  • Updated documentation for changed code.

@Secrus Secrus added the impact/docs Contains or requires documentation changes label Oct 14, 2024
@Secrus Secrus marked this pull request as ready for review October 14, 2024 20:02
@Secrus Secrus requested a review from a team October 14, 2024 20:02
Copy link

github-actions bot commented Oct 14, 2024

Deploy preview for website ready!

✅ Preview
https://website-6enutc4ve-python-poetry.vercel.app

Built with commit 12b301e.
This pull request is being automatically deployed with vercel-action


class EnvActivateCommand(Command):
name = "env activate"
description = "Print the command to activate a virtual environment"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description = "Print the command to activate a virtual environment"
description = "Print the command to activate a virtual environment."

@staticmethod
def quote(command: str, shell: str) -> str:
if shell in ["powershell", "pwsh"] or WINDOWS:
return "{}".format(command.replace("'", "''"))
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this should handle paths with '? However, it does not seem to work (on Windows) and it also does not work for paths with spaces (which is far more common and thereby more important).

On Windows, for cmd you just have to put the command in double quotes (f'"{command}"') and for Powershell you need something like f'& "{command}"'.

@pytest.mark.parametrize(
"shell, command, ext",
(
("pwsh", ".", "Activate.ps1"),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
("pwsh", ".", "Activate.ps1"),
("cmd", ".", "activate"),
("pwsh", ".", "Activate.ps1"),

"Discovered shell doesn't have an activator in virtual environment"
)

def get_activate_command(self, env: Env) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def get_activate_command(self, env: Env) -> str:
def _get_activate_command(self, env: Env) -> str:

I'd add an underscore to make clear it is internal.

return ""

@staticmethod
def quote(command: str, shell: str) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def quote(command: str, shell: str) -> str:
def _quote(command: str, shell: str) -> str:

I'd add an underscore to make clear it's internal

@@ -648,29 +648,6 @@ poetry run my-script

Note that this command has no option.

## shell
Copy link
Member

Choose a reason for hiding this comment

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

Maybe, we should not remove it (now) but write that it has been outsourced into a plugin and we recommend to use env activate instead. (We did not remove export from the docs. I hope this will save us some support requests because some people will look here.)

Comment on lines +81 to +82
`poetry env activate` command prints the activate command to the console. This way you won't leave the current shell.
You can then feed the output to `eval` to activate the environment. This way is the closest to manually activating the environment.
Copy link
Member

Choose a reason for hiding this comment

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

I would restructure the text a bit and do not refer to eval but "the eval command of your shell" (or similar). Proposal:

Suggested change
`poetry env activate` command prints the activate command to the console. This way you won't leave the current shell.
You can then feed the output to `eval` to activate the environment. This way is the closest to manually activating the environment.
The `poetry env activate` command prints the activate command of the virtual environment to the console.
You can run the output command manually or feed it to the eval command of your shell to activate the environment.
This way you won't leave the current shell.

Comment on lines +84 to +86
{{% note %}}
Looking for `poetry shell`? It was moved to a plugin: [`poetry-plugin-shell`](https://github.com/python-poetry/poetry-plugin-shell)
{{% /note %}}
Copy link
Member

Choose a reason for hiding this comment

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

I'd either put this at the beginning or the end and not between the text and the examples how to feed the output to the eval command of different shells.


```bash
$ eval $(poetry env activate)
(test-project-for-test) $ # Virtualenv entered
Copy link
Member

Choose a reason for hiding this comment

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

This line is missing for fish and powershell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/docs Contains or requires documentation changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants