bpo-38344 else needs to be on the same line as the if #16533
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
Lib/venv/scripts/nt/activate.bat
Outdated
| @@ -21,8 +21,7 @@ set PROMPT=__VENV_PROMPT__%PROMPT% | |||
| if defined PYTHONHOME set _OLD_VIRTUAL_PYTHONHOME=%PYTHONHOME% | |||
| set PYTHONHOME= | |||
|
|
|||
| if defined _OLD_VIRTUAL_PATH set PATH=%_OLD_VIRTUAL_PATH% | |||
| else set _OLD_VIRTUAL_PATH=%PATH% | |||
| if defined _OLD_VIRTUAL_PATH set PATH=%_OLD_VIRTUAL_PATH% else set _OLD_VIRTUAL_PATH=%PATH% | |||
There was a problem hiding this comment.
This will never execute the else clause, and if _OLD_VIRTUAL_PATH is defined it will set PATH literally to the evaluation of the string "%_OLD_VIRTUAL_PATH% else set _OLD_VIRTUAL_PATH=%PATH%".
Why again were parentheses stripped out of this script? Whatever the reason, there must be a better way. It's dysfunctional to not be able to write this statement as follows:
if defined _OLD_VIRTUAL_PATH (
set PATH=%_OLD_VIRTUAL_PATH%
) else (
set _OLD_VIRTUAL_PATH=%PATH%
)
There was a problem hiding this comment.
I didn't mention it previously, but in the current form the script errors out:
C:\Program Files\Python38\Lib\venv\scripts\nt>activate.bat
'else' is not recognized as an internal or external command,
operable program or batch file.
But good point on the parenthesis to make it function properly again.
There was a problem hiding this comment.
The parentheses were stripped out because stray parentheses and quotes were breaking the script for people.
Having if defined ... followed by if not defined ... on separate (single) lines is probably the best approach.
| @@ -21,8 +21,11 @@ set PROMPT=__VENV_PROMPT__%PROMPT% | |||
| if defined PYTHONHOME set _OLD_VIRTUAL_PYTHONHOME=%PYTHONHOME% | |||
| set PYTHONHOME= | |||
|
|
|||
| if defined _OLD_VIRTUAL_PATH set PATH=%_OLD_VIRTUAL_PATH% | |||
| else set _OLD_VIRTUAL_PATH=%PATH% | |||
There was a problem hiding this comment.
Please restore the old syntax and replace the else with if not defined ....
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be poked with soft cushions! |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @zooba: please review the changes made to this pull request. |
Lib/venv/scripts/nt/activate.bat
Outdated
| if defined _OLD_VIRTUAL_PATH set PATH=%_OLD_VIRTUAL_PATH% | ||
| else set _OLD_VIRTUAL_PATH=%PATH% | ||
| if defined _OLD_VIRTUAL_PATH set "PATH=%_OLD_VIRTUAL_PATH%" | ||
| if not defined _OLD_VIRTUAL_PATH set "_OLD_VIRTUAL_PATH=%PATH%" |
There was a problem hiding this comment.
Need to lose the quotes as well - they can cause problems if there are mismatched quotes in the variables (and they're not necessary when the statement extends to the end of the line, which is why the original change was necessary).
| @@ -0,0 +1 @@ | |||
| Fix usage of _OLD_VIRTUAL_PATH in activate.bat. Do not use parenthesis in the if statement. Instead of if/else, use if defined/if not defined on separate lines. | |||
There was a problem hiding this comment.
The NEWS item doesn't need to be this detailed - it'll link back to the bug, which links to the PR, which links to the changes for anyone interested. Simply saying "Fix error message in activate.bat" would be fine.
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @zooba: please review the changes made to this pull request. |
|
Thanks @jamesabel for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8. |
|
Sorry @jamesabel and @zooba, I had trouble checking out the |
(cherry picked from commit e310af9) Co-authored-by: James Abel <j@abel.co>
|
GH-16627 is a backport of this pull request to the 3.7 branch. |
|
Thanks! I've merged it as-is, since we need the fix, but we'll leave the bug open for now to consider enhancing the test suite as well. |
|
Thanks @jamesabel for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
(cherry picked from commit e310af9) Co-authored-by: James Abel <j@abel.co>
|
GH-16628 is a backport of this pull request to the 3.8 branch. |
(cherry picked from commit e310af9) Co-authored-by: James Abel <j@abel.co>
In activate.bat the else needs to be on the same line as the if
https://bugs.python.org/issue38344