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

No longer forcibly echo off during windows batch activation #2801

Merged
merged 11 commits into from
Nov 26, 2024

Conversation

wiktorinox
Copy link
Contributor

@wiktorinox wiktorinox commented Nov 22, 2024

During fixing of #2728 new issue has been introduced.
Adding echo off at the beginning of the activate.bat cause this setting to persist after activation.
It doesn't occur when activated in the interactive shell, but it does when activating inside of the Windows batch script - incidentaly that's how Jenkins bat step executes it's contents.
This behaviour is unexpected and activation should not force specific echo on/off setting.

This change removes that echo off while still fixing #2728 - no additional parentheses are displayed when activating in windows batch.

I would love to write tests for that but I have no idea how - the way current tests are written is pretty convoluted and I don't understand them. However, I hope that this change is small enough that tests won't be necessary.

Thanks for contributing, make sure you address all the checklists (for details on how see development documentation)

  • ran the linter to address style issues (tox -e fix)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Test please.

@gaborbernat gaborbernat marked this pull request as draft November 22, 2024 06:20
@wiktorinox
Copy link
Contributor Author

@gaborbernat Like I wrote in the PR - I don't understand how to write the tests in this repository. If you could advise what's the execution flow and what parts are necessary for the test and where the assertions should live, then I would be able to write the tests.

For example for the test for this functionality I need to be able to create a bat file that will contain invocation of activate.bat and then observe output - is that even possible in your framework?

@gaborbernat
Copy link
Contributor

Yes, and you'll see example of if in the test suite.

@wiktorinox
Copy link
Contributor Author

I've added a test for this case.

@wiktorinox wiktorinox marked this pull request as ready for review November 22, 2024 12:37
gaborbernat
gaborbernat previously approved these changes Nov 22, 2024
@gaborbernat gaborbernat enabled auto-merge (squash) November 22, 2024 15:21
@gaborbernat gaborbernat dismissed their stale review November 26, 2024 00:59

The merge-base changed after approval.

@gaborbernat gaborbernat force-pushed the main branch 3 times, most recently from fe26af4 to 7c0a39f Compare November 26, 2024 01:03
@gaborbernat gaborbernat disabled auto-merge November 26, 2024 01:05
@gaborbernat gaborbernat merged commit b3e2b6f into pypa:main Nov 26, 2024
1 of 2 checks passed
@wiktorinox wiktorinox deleted the fix_echo_on_off_winbatch_activate branch November 26, 2024 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants