Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
bpo-37354: Make Powershell Activate.ps1 script static to allow for signing #14967
bpo-37354: Make Powershell Activate.ps1 script static to allow for signing #14967
Changes from 5 commits
57d060e
f15d490
d7e442e
1099c9b
481b7b7
5874721
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 the line that gave me the most nightmares when thinking about how to do this, because the prompt value in the config file is quoted and escaped (basically, it's
repr(prompt)
), so we need to do more processing here.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.
Thanks for the review @zooba!
Agreed that the escaped character problem is a bit tricky in the context of Powershell...
\n
is not really available in Powershell, for instance, but could be replaced with the back-tick-quote `. Finding and replacing all types of escaped characters (newlines, tabs, colors?) might be a bit of a deep dark hole (there be ANSI-escaped coloured dragons there)!Would it be appropriate to add handling of escaped characters in a different issue/PR? I don't expect that the prior version of this Activate.ps1 script handled this problem particularly well either (a brief test with the latest 3.8 beta shows a few hiccups right away).
Of course, I would be most happy to tackle that problem next!
What sample input would show the shortcoming of this implementation, so that I can write some unit tests and fix it correctly?
Just from thinking about it briefly I can think up input that contains:
\n
,\r\n
)\t
)\u001b[31m
, ... ,\u001b[0m
)Ideally I'd love to show something that would have the class of input we need to be concerned about working with, and have it handled at least as well as the prior implementation handled it.
A specific scenario that is trivial to setup and play with, for example the newline escaped character
\n
in the prompt:...and then in bash...
...as we can see, the
\\n
being stored in thepyvenv.cfg
is written to the prompt verbatim in Powershell with this change (and is also in the latest 3.8 beta release).Note: it is possible in Powershell to replace some escaped character's
\\
with a ` and it will work. (But definitely not colours, those are weird special creatures!).