-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix poetry env activate for Bash on Windows
#10415
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
Reviewer's GuideThis PR refactors the activation command generation to support various shells, simplifies the quoting logic, and adds comprehensive tests for Windows and Unix shells. Sequence Diagram for Activation Command Generation in
|
| Change | Details | Files |
|---|---|---|
| Revise shell detection and activation command assembly |
|
src/poetry/console/commands/env/activate.py |
| Simplify script path quoting logic |
|
src/poetry/console/commands/env/activate.py |
| Extend test coverage for env activate across shells |
|
tests/console/commands/env/test_activate.py |
Possibly linked issues
poetry env activatedoes not work in Bash for Windows #10395: PR modifies activate script to outputsourceand path for Bash on Windows, fixing the issue.poetry env activatedoes not work in Bash for Windows #10395: The PR modifies the activation command logic for Windows shells, fixing the issue wherepoetry env activatefailed.poetry env activatedoes not work in Bash for Windows #10395: PR fixespoetry env activateWindows activation script handling for shells like PowerShell as described in the issue.
Tips and commands
Interacting with Sourcery
- Trigger a new review: Comment
@sourcery-ai reviewon the pull request. - Continue discussions: Reply directly to Sourcery's review comments.
- Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with@sourcery-ai issueto create an issue from it. - Generate a pull request title: Write
@sourcery-aianywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai titleon the pull request to (re-)generate the title at any time. - Generate a pull request summary: Write
@sourcery-ai summaryanywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment@sourcery-ai summaryon the pull request to
(re-)generate the summary at any time. - Generate reviewer's guide: Comment
@sourcery-ai guideon the pull
request to (re-)generate the reviewer's guide at any time. - Resolve all Sourcery comments: Comment
@sourcery-ai resolveon the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore. - Dismiss all Sourcery reviews: Comment
@sourcery-ai dismisson the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai reviewto trigger a new review!
Customizing Your Experience
Access your dashboard to:
- Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others. - Change the review language.
- Add, remove or edit custom review instructions.
- Adjust other review settings.
Getting Help
- Contact our support team for questions or feedback.
- Visit our documentation for detailed guides and information.
- Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.
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.
Hey @PunVas - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
The \poetry env activate\ command was checking OS instead of shell type, causing bash on Windows to output only the quoted path instead of \source /path/to/activate\. This change: - Checks shell type instead of OS when determining output format - Ensures bash on Windows gets 'source' command prefix - Maintains correct behavior for PowerShell and cmd - Simplifies quoting logic Fixes python-poetry#10395
The \poetry env activate\ command was checking OS instead of shell type, causing bash on Windows to output only the quoted path instead of \source /path/to/activate\. This change: - Checks shell type instead of OS when determining output format - Ensures bash on Windows gets 'source' command prefix - Maintains correct behavior for PowerShell and cmd - Simplifies quoting logic Fixes python-poetry#10395
The \poetry env activate\ command was checking OS instead of shell type, causing bash on Windows to output only the quoted path instead of \source /path/to/activate\. This change: - Checks shell type instead of OS when determining output format - Ensures bash on Windows gets 'source' command prefix - Maintains correct behavior for PowerShell and cmd - Simplifies quoting logic Fixes python-poetry#10395
The \poetry env activate\ command was checking OS instead of shell type, causing bash on Windows to output only the quoted path instead of \source /path/to/activate\. This change: - Checks shell type instead of OS when determining output format - Ensures bash on Windows gets 'source' command prefix - Maintains correct behavior for PowerShell and cmd - Simplifies quoting logic Fixes python-poetry#10395
The \poetry env activate\ command was checking OS instead of shell type, causing bash on Windows to output only the quoted path instead of \source /path/to/activate\. This change: - Checks shell type instead of OS when determining output format - Ensures bash on Windows gets 'source' command prefix - Maintains correct behavior for PowerShell and cmd - Simplifies quoting logic Fixes python-poetry#10395
The \poetry env activate\ command was checking OS instead of shell type, causing bash on Windows to output only the quoted path instead of \source /path/to/activate\. This change: - Checks shell type instead of OS when determining output format - Ensures bash on Windows gets 'source' command prefix - Maintains correct behavior for PowerShell and cmd - Simplifies quoting logic Fixes python-poetry#10395
|
@Secrus @sdispater |
|
@PunVas Please take a look at "Files changed". For a fix like that, only a couple of files should be involved. It looks like you committed your pycache. |
poetry env activate for Bash on Windows
|
@radoering hi have added pycache to .gitignore and removed existing ones. please review and tell if any further change is required. |
5101a51 to
ce23789
Compare
| if WINDOWS and shell not in ["powershell", "pwsh", "cmd"]: | ||
| return shlex.quote(command) | ||
| elif shell in {"powershell", "pwsh", "cmd"}: | ||
| return command | ||
| else: | ||
| return shlex.quote(command) |
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.
| if WINDOWS and shell not in ["powershell", "pwsh", "cmd"]: | |
| return shlex.quote(command) | |
| elif shell in {"powershell", "pwsh", "cmd"}: | |
| return command | |
| else: | |
| return shlex.quote(command) | |
| if shell in {"powershell", "pwsh", "cmd"}: | |
| return command | |
| return shlex.quote(command) |
This is equivalent, isn't it?
| ), | ||
| ) | ||
| @pytest.mark.skipif(not WINDOWS, reason="Only Windows shells") | ||
| def test_env_activate_windows_shells_get_quoted_path_only( |
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 is more or less just a copy of test_env_activate_prints_correct_script_on_windows, isn't it?
| ("sh", "source", ""), | ||
| ), | ||
| ) | ||
| def test_env_activate_unix_shells_get_command_with_path( |
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 looks like a copy of test_env_activate_prints_correct_script that also works for Windows. Can we just edit the original test instead of creating a new one?
| assert line == expected | ||
|
|
||
|
|
||
| def test_env_activate_bash_on_windows_gets_source_command( |
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 is already covered by test_env_activate_unix_shells_get_command_with_path, isn't it?
| if shell == "cmd" and not WINDOWS: | ||
| fallback_script = env.bin_dir / "activate" | ||
| if fallback_script.exists(): | ||
| return f"source {self._quote(str(fallback_script), 'bash')}" |
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.
What is this for? I know that Powershell exists for Linux but cmd? Does this cover a real-world use case? Does a test fail if we remove this?
Pull Request Check List
Resolves: #10395
description
this fixes
poetry env activatenot working properly in Bash for windows envs.if you find any problems in my my please tell me. i will try to implemet that because its my first opensource contributionn
Summary by Sourcery
Fix the
poetry env activatecommand generation to properly format and quote activation scripts for various shells (including Bash on Windows, PowerShell, cmd, Unix shells, and unknown shells).Bug Fixes:
env activatefunctionality in Bash on Windows by prependingsourceand avoiding incorrect quoting.&and cmd scripts are invoked without extra flags.Enhancements:
shellingham.detect_shell._get_activate_commandto dispatch based on shell type rather than a generic Windows flag._quoteto apply quoting only when necessary and return unmodified commands for PowerShell and cmd.Tests: