-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Updating code_utils.py to solve issue #1747 #1758
Conversation
Updated the powershell command to pwsh
added a split to handle powershell in the first condition as well
added "pwsh" as a command option in lang variable
Co-authored-by: Eric Zhu <ekzhu@users.noreply.github.com>
fixed formatting
defined a function to detect whether 'powershell' or 'pwsh' works and accordingly use the one that works
fixed formatting
added a unit test for get_powershell_command function in code_utils.py
fixed formatting
fixed formatting
Co-authored-by: Chi Wang <wang.chi@microsoft.com>
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!
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 so much for the PR! I made two lint comments regarding the changes.
Happy to approve this PR anytime.
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.
LGTM~
Thanks for resolving the issue!
No problem! How do I merge this PR now? It still says merging is blocked |
* Update code_utils.py Updated the powershell command to pwsh * Update code_utils.py added a split to handle powershell in the first condition as well * Update local_commandline_code_executor.py added "pwsh" as a command option in lang variable * Update autogen/coding/local_commandline_code_executor.py Co-authored-by: Eric Zhu <ekzhu@users.noreply.github.com> * Update code_utils.py * Update code_utils.py fixed formatting * Update code_utils.py defined a function to detect whether 'powershell' or 'pwsh' works and accordingly use the one that works * Update code_utils.py fixed formatting * Update and rename test_code.py to test_code_utils.py added a unit test for get_powershell_command function in code_utils.py * Update test_code_utils.py fixed formatting * Update test_code_utils.py fixed formatting * Update autogen/code_utils.py Co-authored-by: Chi Wang <wang.chi@microsoft.com> * solved issue microsoft#1747 * updated unit test * fixed formatting * fixed formatting * fixed formatting * fixed a bug * removed extra return None and removed redundant comments --------- Co-authored-by: Eric Zhu <ekzhu@users.noreply.github.com> Co-authored-by: Chi Wang <wang.chi@microsoft.com>
Why are these changes needed?
These changes are needed since the print statement print("Neither powershell nor pwsh is installed.") was being noisy and unnecessarily repetitive. It was giving this warning for linux systems as well thinking that there is something wrong with the system because neither powershell nor pwsh is installed but linux systems dont have either anyway so this warning is unnecessary for non-windows operating systems.
I added a condition so that the warning only gets issued if the OS is detected as windows since if it is any other OS, powershell or pwsh need not be there, some other terminal may be used like bash for linux for example.
I also replace print with logging.warning so that the warning isnt given repetitively
Related issue number
Closes #1747
Checks