Skip to content

Add new function shell_tools.run #5528

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

Merged
merged 5 commits into from
Jun 16, 2022

Conversation

pavoljuhas
Copy link
Collaborator

Add shell_tools.run, a thin wrapper to subprocess.run with
a few extra arguments for showing the executed commands.
It also supplies updated default values for subprocess.run
to approximate behavior of run_cmd and run_shell functions.
The purpose of this function is to replace the run_cmd and
run_shell functions in the shell_tools module.

Partially implements #4394

A thin wrapper to subprocess.run with an extra options to log
command arguments to stderr.
Default the `capture_output`, `check`, and `text` arguments to True
to approximate behavior of run_cmd.
@pavoljuhas pavoljuhas requested review from a team, vtomole and cduck as code owners June 15, 2022 18:31
@pavoljuhas pavoljuhas requested a review from viathor June 15, 2022 18:31
@CirqBot CirqBot added the size: M 50< lines changed <250 label Jun 15, 2022
@pavoljuhas
Copy link
Collaborator Author

Premature PR - please hold with review...

@pavoljuhas pavoljuhas force-pushed the add-shell_tools.run branch from 3419bc4 to 9e9d9a5 Compare June 15, 2022 19:19
Copy link
Collaborator

@dabacon dabacon left a comment

Choose a reason for hiding this comment

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

LGTM

One thought on documentation, but however you decide is good.

**subprocess_run_kwargs,
) -> subprocess.CompletedProcess:
"""Call subprocess.run with an option to log executed command to stderr.

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe something to distinguish why you would use this and not run_cmd below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ack. I actually plan to remove run_cmd and run_shell in a followup PR and only use the run() function.
This PR is 1 of 4 for #4394.

Thanks for quick review.

@pavoljuhas pavoljuhas added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jun 16, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jun 16, 2022
@CirqBot CirqBot merged commit f4f9ac6 into quantumlib:master Jun 16, 2022
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jun 16, 2022
@pavoljuhas pavoljuhas deleted the add-shell_tools.run branch June 16, 2022 23:08
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Add shell_tools.run, a thin wrapper to subprocess.run with
a few extra arguments for showing the executed commands.
It also supplies updated default values for subprocess.run
to approximate behavior of run_cmd and run_shell functions.
The purpose of this function is to replace the run_cmd and
run_shell functions in the shell_tools module.

Partially implements quantumlib#4394
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants